Make WordPress Core

Opened 10 months ago

Closed 5 months ago

Last modified 3 months ago

#59647 closed task (blessed) (fixed)

Test tool and unit test improvements for 6.5

Reported by: hellofromtonya's profile hellofromTonya Owned by:
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Previously:

This ticket is for various fixes and improvements in PHPUnit tests that don't have a more specific ticket, as well as general improvements to the GitHub Actions workflows that run automated testing.

Change History (46)

This ticket was mentioned in PR #5100 on WordPress/wordpress-develop by @johnbillion.


10 months ago
#1

  • Keywords has-patch has-unit-tests added

Trac ticket:
https://core.trac.wordpress.org/ticket/59647
https://core.trac.wordpress.org/ticket/58955

This changes several instances of test skipping to test failures, and removes some unnecessary function_exists() checks for compat functions.

If any of the checks in these test aren't satisfied then the test should fail rather than be skipped.

In addition, I removed some multisite test skipping which is redundant as it's handled by the ms-required and ms-excluded test group handling.

Previously:

#2 @SergeyBiryukov
10 months ago

In 56969:

Tests: Remove some unnecessary function_exists() checks for compat functions.

Each of these functions already has a separate test for availability.

If any of them are unavailable, then the test should fail rather than be skipped.

Follow-up to [52038], [52039], [52040].

Props johnbillion.
See #59647.

#3 @SergeyBiryukov
10 months ago

In 56971:

Tests: Improve the @group annotation accuracy and consistency.

Includes removing .php from some older group names, because most of the groups are no longer named based on the file containing the function, and sometimes functions move around, making the file-based group name inaccurate.

Props afercia, aristath, poena, SergeyBiryukov.
See #59647.

#4 @SergeyBiryukov
10 months ago

In 57008:

Tests: Remove some unnecessary multisite test skipping.

These checks are redundant, as the skipping already handled by the ms-required and ms-excluded groups.

Follow-up to [36740], [36741], [38705], [40520], [51415].

Props johnbillion.
See #59647.

#5 @SergeyBiryukov
9 months ago

In 57011:

Tests: Use a @requires annotation for readonly() function test.

The function is only defined by WordPress core on PHP < 8.1.

Follow-up to [51586].

See #59647.

#6 @jorbin
9 months ago

#59806 was marked as a duplicate.

#7 @peterwilsoncc
9 months ago

In 57057:

Build/Test Tools: Fix group for wp_unique_prefixed_id() tests.

Change the group from functions.php to functions to match other tests.

See #59647.

#8 follow-up: @peterwilsoncc
9 months ago

In 57058:

Build/Test Tools: Fix group for wp_unique_prefixed_id() tests.

Change the group from functions.php to functions to match other tests.

Reviewed by jorbin.
Merges [57057] to the 6.4 branch.

See #59647, #58955.

#9 in reply to: ↑ 8 @SergeyBiryukov
9 months ago

In 57058:

Build/Test Tools: Fix group for wp_unique_prefixed_id() tests.

Change the group from functions.php to functions to match other tests.

Reviewed by jorbin.
Merges [57057] to the 6.4 branch.

@peterwilsoncc The functions.php group was only renamed to functions for trunk in [56971], it's still functions.php in the 6.4 branch, so [57057] should probably also be in trunk only.

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#10 @peterwilsoncc
9 months ago

@SergeyBiryukov Thanks, I'll fix that now.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


9 months ago

#12 @peterwilsoncc
9 months ago

In 57059:

Build/Test Tools: Revert [57058].

Revert group name change in the 6.4 branch as the functions.php group was renamed functions after the branch was forked.

Props SergeyBiryukov.
See #59647, #58955.

#13 @swissspidy
8 months ago

In 57143:

Test: Run database upgrades between performance test steps.

Prevents pending upgrades from blocking tests when checking out previous builds.

Props mukesh27.
See #59647.

#14 @SergeyBiryukov
8 months ago

In 57146:

Tests: Update _wp_timezone_choice_usort_callback() tests for consistency.

  • Use the same @group annotation as the other tests.
  • Use assertSame() to verify the type of the result.
  • Use data_ prefix for the data provider.
  • Use named data set in the data provider. This makes the output when using the --testdox option more descriptive and is helpful when trying to debug which data set from a data provider failed the test.
  • Other minor corrections.

Reference: Core Handbook: Writing PHP Tests: Repetitive Tests.

Follow-up to [57145].

See #59953, #59647.

This ticket was mentioned in PR #5849 on WordPress/wordpress-develop by @hellofromTonya.


7 months ago
#15

Reorganizes the hooks tests to align to coding standards and consistency within the test suite.

  • [X] Moves WP_Hooks method tests to into separate directory <GroupName>/<className>/<methodName>.php.
  • [X] Splits the function tests in actions.php and filters.php into separate test classes within the tests/hooks/ directory.

Trac ticket: https://core.trac.wordpress.org/ticket/59647

#16 @SergeyBiryukov
7 months ago

In 57278:

Tests: Correct the @group annotation in some tests.

Follow-up to [56971], [57146].

See #59647.

#17 @SergeyBiryukov
7 months ago

In 57284:

Tests: Move wp_parse_list() tests to their own file.

This aims to make the tests more discoverable and easier to expand.

Follow-up to [44546].

See #59647.

This ticket was mentioned in PR #5987 on WordPress/wordpress-develop by @jonsurrell.


6 months ago
#18

Trac ticket: https://core.trac.wordpress.org/ticket/59647

This ticket was mentioned in PR #5987 on WordPress/wordpress-develop by @jonsurrell.


6 months ago
#19

Trac ticket: Core-59647

Rename $p variable to $processor in tests for clarity (https://github.com/WordPress/wordpress-develop/pull/5975#discussion_r1472011462).

Use static data providers. A mix of static and non-static data providers were used in HTML API tests.
Data providers are required to be static in the next PHPUnit version and there's no harm in using
them consistently now.

#20 @dmsnell
6 months ago

In 57508:

HTML API: Test cleanup

Rename $p variable to $processor in tests for clarity.

Use static data providers. A mix of static and non-static data providers were
used in HTML API tests. Data providers are required to be static in the next
PHPUnit version and there's no harm in using them consistently now.

Follow-up to [57507]

Props jonsurrell
See #59647

This ticket was mentioned in PR #5987 on WordPress/wordpress-develop by @jonsurrell.


6 months ago
#22

Trac ticket: Core-59647

Rename $p variable to $processor in tests for clarity (https://github.com/WordPress/wordpress-develop/pull/5975#discussion_r1472011462).

Use static data providers. A mix of static and non-static data providers were used in HTML API tests.
Data providers are required to be static in the next PHPUnit version and there's no harm in using
them consistently now.

This ticket was mentioned in PR #5998 on WordPress/wordpress-develop by @peterwilsoncc.


6 months ago
#23

Prevent HTTP failures connecting to WordPress.org from triggering test failures.

The WP_REST_Plugins_Controller_Test runs in the standard test suite rather than the HTTP request test suite so this should stop the test from failing when wordpress.org connections are unavailable.

Trac ticket: https://core.trac.wordpress.org/ticket/59647

#24 @peterwilsoncc
6 months ago

In 57531:

Build/Test Tools: Mock plugin API response in WP_REST_Plugins_Controller_Test.

Avoid false test failures due to network conditions in the WP_REST_Plugins_Controller_Test class. This mocks HTTP responses from the plugin information endpoint for the link-manager plugin.

Props: peterwilsoncc, costdev.
See #59647.

@desrosj commented on PR #6031:


6 months ago
#27

So I've run the coverage report on all 8.x PHP versions. There are more warnings on PHP 8.x versions, and more the higher you go because of the current beta support. Here's how I found that translating to test coverage numbers:

PHP Version | Lines % | Functions and Methods | Classes and Traits
-- | -- | -- | --
7.4 | 44.44 | 34.74 | 8.72
8 | 44.44 | 34.68 | 8.72
8.1 | 44.44 | 34.87 | 8.72
8.2 | 44.39 | 34.82 | 8.72
8.3 | 44.23 | 34.76 | 8.72

There are slight fluctuations in the overall coverage for lines, and functions and methods, but nothing greater than a few tenths of a percent. There will be a small change after committing this, but I think removing the variable all together allowing the version tagged latest in Docker to be used (currently PHP 8.2) is the right way forward.

#28 @desrosj
6 months ago

In 57551:

Build/Test Tools: Pin a specific commit for Hosting Test Reporter.

An upstream change in the Hosting Test Reporter is causing failures in the PHPUnit workflow. This temporarily pins an older hash to the step that checks the reporter out.

Props youknowriad.
See #59647.

#29 @desrosj
6 months ago

In 57552:

Build/Test Tools: Revert [57551].

Problematic changes to the test reporter were merged upstream. This specific SHA also did not fix the issue.

Props javiercasares.
See #59647.

#30 @desrosj
6 months ago

In 57566:

Build/Test Tools: Unpin PHP 7.4 from the test coverage workflow.

Since being introduced, the GitHub Actions workflow responsible for generating and submitting a test coverage report has used PHP 7.4. At the time, there were some issues with running the test suite on PHP 8.0+ which could have resulted in inaccurate reporting.

The test suite and WordPress in general are much more stable on 8.x now. The test coverage report should now be generated using the Docker container tagged latest (currently 8.2).

This will result in a very small decrease in the percentage of lines covered by test (-0.05%), but a slight increase in the percentage of functions and methods covered (+0.08%).

Props johnbillion.
See #59647.

#32 @swissspidy
6 months ago

  • Type changed from enhancement to task (blessed)

This ticket was mentioned in PR #6145 on WordPress/wordpress-develop by @costdev.


6 months ago
#33

Previously, the tests for WP_Plugin_Dependencies::get_dependency_names() performed an API request to WordPress.org. If an HTTP failure occurred when connecting to WordPress.org, this could trigger test failures.

This mocks the API response to prevent HTTP failures from triggering test failures.

#34 @costdev
6 months ago

In 57674:

Tests: Mock API response in Plugin Dependencies tests.

Previously, the tests for WP_Plugin_Dependencies::get_dependency_names() performed an API request to WordPress.org. If an HTTP failure occurred when connecting to WordPress.org, this could trigger test failures.

This mocks the API response to prevent HTTP failures from triggering test failures.

Follow-up to [57545].

Props swissspidy, peterwilsoncc, costdev.
See #59647.

@costdev commented on PR #6145:


6 months ago
#35

Committed in r57674.

This ticket was mentioned in PR #6148 on WordPress/wordpress-develop by @costdev.


6 months ago
#36

The test_readme_mariadb_version() test method has been experiencing HTTP failures that results in test and CI failures.

This switches to mariadb.org which may result in more stable test runs.

Some additional safety assertions are also included.

#37 @costdev
6 months ago

In 57684:

Tests: Query mariadb.org instead of mariadb.com in README test.

The test_readme_mariadb_version() test method has been experiencing HTTP failures that result in test and CI failures.

This switches to mariadb.org which may result in more stable test runs.

Some additional safety assertions are also included.

Props hellofromTonya, SergeyBiryukov, costdev.
See #59647.

@costdev commented on PR #6148:


6 months ago
#38

Committed in r57684.

#39 @hellofromTonya
6 months ago

In 57689:

Build/Test Tools: Revert r57684.

Changes to the test_readme_mariadb_version() test were made in an effort to stabilize the connection tests and CI failures. After the changeset, all multisite tests CI jobs connected and passed, but oddly all single site tests CI jobs repeatedly and consistently failed.

Reverting to unblock the CI jobs while continued investigation happens.

See #59647.

#40 @SergeyBiryukov
5 months ago

In 57733:

Tests: Correct capitalization and fix typos in some test class names.

Follow-up to [57060], [57718], [57725], [57726], [57727], [57728].

See #59647.

#41 @swissspidy
5 months ago

In 57735:

Tests: Address capitalization and docblock inconsistencies in some test class names.

Follow-up to [57060], [57718], [57725], [57726], [57727], [57728], [57733].

Props swissspidy, costdev.
See #59647.

#42 @SergeyBiryukov
5 months ago

In 57737:

Tests: Expand wp_parse_id_list() unit tests.

Includes:

  • Moving pre-existing wp_parse_id_list() tests to their own file.
  • Merging new and pre-existing wp_parse_slug_list() tests.
  • Using named data provider in wp_parse_list() tests.

Follow-up to [25170], [40044], [44546], [57284], [57725].

Props pbearne, mukesh27, SergeyBiryukov.
Fixes #60218. See #60217, #59647.

#43 @swissspidy
5 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Closing for now since 6.5 has been branched, so any new updates like this can happen in trunk for 6.6.

This ticket was mentioned in PR #6505 on WordPress/wordpress-develop by @johnbillion.


3 months ago
#44

I noticed when trying to run composer lint:errors on my local installation that the process would run out of memory:

..........E....
Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 90366752 bytes) in vendor/squizlabs/php_codesniffer/src/Runner.php on line 547

This appears to be caused by phpcs trying to scan the *.i18n.php translation files in the wp-content/languages directory, of which I have quite a few due to testing this functionality.

These files shouldn't be scanned by phpcs so can be ignored.

#45 @johnbillion
3 months ago

In 58107:

Build/Test Tools: Exclude PHP translation files from phpcs linting.

Translation files in this directory are automatically generated and are not subject to any linting considerations.

See #59647

Note: See TracTickets for help on using tickets.