WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 3 weeks ago

#53363 new task (blessed)

Test tool and unit test improvements for 5.9

Reported by: desrosj Owned by:
Milestone: 5.9 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.

Attachments (13)

53363-01.patch (8.4 KB) - added by jrf 3 months ago.
Annotate when tests do deliberately do not perform assertions, so the test won't be marked as risky
53363-02-use-assertfileisreadable.patch (1.8 KB) - added by jrf 3 months ago.
Use more appropriate assertion [1] - this assertion is available PHPUnit cross-version, so can already be used
53363-03-use-assertdirectoryexists.patch (1.0 KB) - added by jrf 3 months ago.
Use more appropriate assertion [2] - this assertion is available PHPUnit cross-version, so can already be used
53363-04-fix-incorrectly-named-test-class.patch (992 bytes) - added by jrf 3 months ago.
A concrete test class should be suffixed with Test, not UnitTestCase(s)
53363-05-custom-assertions-should-have-msg-param.patch (6.6 KB) - added by jrf 3 months ago.
Build/Tests: custom assertions should always have a $message parameter All assertions in PHPUnit have a $message parameter. Setting this parameter allows to distinguish which assertion is failing when a test runs multiple assertion, making debugging of the tests easier. The WP custom assertions which are included in the WP_UnitTestCase_Base class should also allow for this $message parameter. This patch adds this - optional - parameter for those assertions which were missing it.
53363-GH-1519-move-incorrectly-placed-test.patch (13.8 KB) - added by jrf 3 months ago.
The three commits of GH PR 1519 as one patch ready for commit
53363-06-fix-EOL-test-bootstrap.patch (4.0 KB) - added by jrf 2 months ago.
Build/Tests: fix message display in test bootstrap Any messages to the user which are echo-ed out in the test bootstrap will generally display on a command-line interface. The *nix specific "\n" line ending will be ignored on Windows, making the messages less readable. For new lines in CLI messages, PHP_EOL should be used instead. This was already done in a few places in the script, but not consistently so. Fixed now.
53363-07-add-schema-to-config.patch (1.5 KB) - added by jrf 2 months ago.
PHPUnit config files: add schema reference The current config files validate against the PHPUnit XSD schema for config files for PHPUnit 5.7 - 9.2. The schema was changes in PHPUnit 9.3 and the filter and logging configuration deprecated in favour of coverage and a different format for logging. By setting the schema, deprecation notices about the use of an outdated schema when running on PHPUnit 9.3+ are prevented.
53363-08-testcases-should-be-abstract.patch (1.5 KB) - added by jrf 2 months ago.
Build/Tests: testcases should be abstract TestCases which are intended to be extended and not run directly, should be abstract.
53363-06-follow-up-unicode-char.patch (1.0 KB) - added by jrf 2 months ago.
Tests/bootstrap: fix unicode char having slipped in Follow up on [51581]
53363-09-test-bootstrap-improve-error-msg.patch (2.2 KB) - added by jrf 7 weeks ago.
Tests bootstrap: improve an outdated error condition The PHPUnit tests are/should generally be run on the src directory, so changes just made can be tested. While testing via the build directory is also still supported, this is not the default. This was last changed in [50441], but that commit did not remove/update the error message thrown by the test bootstrap file. This last change also did not take into account that that change meant that people had to update their own wptests-config.php file to match the change made in Core and would otherwise still get the outdated error message. This commit changes the messaging in the test bootstrap file to be more in line with the current reality, while still accounting for the fact that tests can be run from both src as well as build`.
53363-10-custom-assertions-more-stabilizing.patch (4.0 KB) - added by jrf 7 weeks ago.
WP_UnitTestCase_Base: more improvements to custom assertions The assertEqualFields() method expects an object and a fields array as inputs and subsequently approaches the received parameters as such, but did not verify whether the received parameters are of the expected types. Along the same lines, the assertSameSets(), assertEqualSets(), assertSameSetsWithIndex() and the assertEqualSetsWithIndex() methods all expect arrays for both the actual as well as the expected values and uses the array function [k]sort() on both, but never verified that the received inputs were actually arrays, which could lead to PHP errors on the sorting function calls.
53363-11-custom-assertions-improve-assertDiscardWhitespace.patch (2.0 KB) - added by jrf 7 weeks ago.
WP_UnitTestCase_Base::assertDiscardWhitespace(): stabilize the custom assertion The assertDiscardWhitespace() method uses assertEquals() under the hood, meaning that in reality any type of actual/expected value should be accepted by the function. Fixed the documentation to reflect that. At the same time, only strings can contain whitespace differences, so the whitespace replacement should only be done when string values are passed. This will prevent potential passing null to non-nullable errors on PHP 8.1, if either of the inputs would turn out to be null.

Download all attachments as: .zip

Change History (103)

#1 @SergeyBiryukov
3 months ago

In 51333:

Tests: Move loading compatibility layers for PHPUnit 6+ and 7.5+ closer together.

Follow-up to [40536], [44701], [50986].

See #53363.

#2 @SergeyBiryukov
3 months ago

In 51335:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( is_array( ... ) ) with assertIsArray() to use native PHPUnit functionality.

Follow-up to [51331].

See #53363.

#3 @SergeyBiryukov
3 months ago

In 51337:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( is_object( ... ) ) with assertIsObject() to use native PHPUnit functionality.

Follow-up to [51331], [51335].

See #53363.

#4 @SergeyBiryukov
3 months ago

In 51367:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertSame( [number], count( ... ) ) with assertCount() to use native PHPUnit functionality.

Follow-up to [51335], [51337].

See #53363.

#5 @SergeyBiryukov
3 months ago

In 51368:

Coding Standards: Fix WPCS issues in [51367].

This fixes an "Expected 1 space after comma in argument list; 2 found" error.

See #53363.

#6 @SergeyBiryukov
3 months ago

In 51397:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( isset( ... ) ) with assertArrayHasKey() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367].

See #53363.

#7 @SergeyBiryukov
3 months ago

In 51403:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( empty( ... ) ) with assertEmpty() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397].

See #53363.

#8 @SergeyBiryukov
3 months ago

In 51404:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( in_array( ... ) ) with assertContains() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403].

Props hellofromTonya, jrf, SergeyBiryukov.
Fixes #53123. See #53363.

#9 @SergeyBiryukov
3 months ago

In 51405:

Coding Standards: Fix WPCS issue in [51404].

This fixes an "Expected 1 spaces before closing parenthesis; 0 found" error.

See #53363.

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


3 months ago

#11 @SergeyBiryukov
3 months ago

In 51436:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( is_a( ... ) ) with assertInstanceOf() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404].

See #53363.

#12 @SergeyBiryukov
3 months ago

In 51438:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( is_numeric( ... ) ) with assertIsNumeric() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436].

See #53363.

#13 @SergeyBiryukov
3 months ago

In 51448:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( is_string( ... ) ) with assertIsString() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436], [51438].

See #53363.

#14 @SergeyBiryukov
3 months ago

In 51449:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertSame( 0, strpos( ... ) ) with assertStringStartsWith() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436], [51438], [51448].

See #53363.

#15 @SergeyBiryukov
3 months ago

In 51450:

Tests: Correct the test for autosaving a post with Ajax.

wp_autosave() only updates drafts and auto-drafts created by the current user if the post is not locked.

As a result of previous Ajax test refactoring, setting the current user and creating a test post ended up in different methods, with the user being set after the post is already created.

This resulted in the test post being created with the post_author field set to zero, and the current user check in wp_autosave() failed. Instead of updating the original post as the test intended, it created a new autosave.

The test only passed accidentally due to assertGreaterThanOrEqual() not performing a strict type check.

Follow-up to [26995], [35311].

See #53363.

#16 @SergeyBiryukov
3 months ago

In 51451:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( strpos( ... ) > 0 ) with assertStringContainsString() to use native PHPUnit functionality.

Going forward, these methods introduced in PHPUnit 7.5 should be used for similar assertions:

  • assertStringContainsString()
  • assertStringNotContainsString()

As WordPress currently uses PHPUnit 5.7.x to run tests on PHP 5.6, polyfills for these methods are now added to the WP_UnitTestCase class for PHPUnit < 7.5.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436], [51438], [51448], [51449].

See #53363.

#17 @SergeyBiryukov
3 months ago

In 51452:

Tests: Require the WP_REST_Test_Controller class in WP_REST_Controller tests.

This avoids a "Class not found" PHP fatal error when running these tests separately.

Follow-up to [38832].

See #53363.

#18 @SergeyBiryukov
3 months ago

In 51453:

Tests: Use more appropriate assertions in rest_sanitize_request_arg() tests.

This replaces instances of assertSame( true, ... ) with assertTrue() to use native PHPUnit functionality.

Follow-up to [38832].

See #53363.

#19 @SergeyBiryukov
3 months ago

In 51454:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertTrue( ... > 0 ) with assertGreaterThan() to use native PHPUnit functionality.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436], [51438], [51448], [51449], [51451], [51453].

See #53363.

#20 @SergeyBiryukov
3 months ago

In 51461:

Tests: Use more appropriate assertions in various tests.

This replaces instances of assertFalse( stripos( ... ) ) with assertStringNotContainsString() or assertStringNotContainsStringIgnoringCase() to use native PHPUnit functionality.

Going forward, these methods introduced in PHPUnit 7.5 should be used for similar assertions:

  • assertStringContainsString()
  • assertStringContainsStringIgnoringCase()
  • assertStringNotContainsString()
  • assertStringNotContainsStringIgnoringCase()

As WordPress currently uses PHPUnit 5.7.x to run tests on PHP 5.6, polyfills for these methods are now added to the WP_UnitTestCase class for PHPUnit < 7.5.

Follow-up to [51335], [51337], [51367], [51397], [51403], [51404], [51436], [51438], [51448], [51449], [51451], [51453], [51454].

See #53363.

#21 @SergeyBiryukov
3 months ago

In 51462:

Tests: Replace assertContains() with assertStringContainsString() when used with strings.

Using the assertContains() and assertNotContains() methods with string haystacks was deprecated in PHPUnit 8 and removed in PHPUnit 9.

While WordPress test suite currently only supports PHPUnit up to 7.5.x, this allows us to switch to newer assertions ahead of adding full support for PHPUnit 8+.

These methods introduced in PHPUnit 7.5 should be used as an alternative:

  • assertStringContainsString()
  • assertStringContainsStringIgnoringCase
  • assertStringNotContainsString()
  • assertStringNotContainsStringIgnoringCase

As WordPress currently uses PHPUnit 5.7.x to run tests on PHP 5.6, polyfills for these methods were added to the WP_UnitTestCase class for PHPUnit < 7.5.

Follow-up to [51331], [51451], [51461].

Props jrf, dd32, SergeyBiryukov.
See #53363, #46149.

@jrf
3 months ago

Annotate when tests do deliberately do not perform assertions, so the test won't be marked as risky

@jrf
3 months ago

Use more appropriate assertion [1] - this assertion is available PHPUnit cross-version, so can already be used

@jrf
3 months ago

Use more appropriate assertion [2] - this assertion is available PHPUnit cross-version, so can already be used

#23 follow-up: @jrf
3 months ago

Note: both methods used in the above two patches were introduced in PHPUnit 5.6.0. As the test runs on PHPUnit 5.x use PHPUnit 5.7.27, this shouldn't be an issue, though the test bootstrap.php file does give the impression that PHPUnit 5.4 is still supported (though I don't understand why we'd want to support anything but the last version of the PHPUnit 5.x series).

@jrf
3 months ago

A concrete test class should be suffixed with Test, not UnitTestCase(s)

@jrf
3 months ago

Build/Tests: custom assertions should always have a $message parameter All assertions in PHPUnit have a $message parameter. Setting this parameter allows to distinguish which assertion is failing when a test runs multiple assertion, making debugging of the tests easier. The WP custom assertions which are included in the WP_UnitTestCase_Base class should also allow for this $message parameter. This patch adds this - optional - parameter for those assertions which were missing it.

#24 @jrf
3 months ago

Putting a hold on patch 53363-01.patch /cc @johnbillion

There is something weird going on with this.

  • If the tests are run with WP_RUN_CORE_TESTS set to 0, those tests are marked as "risky" due to not performing any assertions. The patch would fix that.
  • However, if the tests are run with WP_RUN_CORE_TESTS set to 1 + this patch applied, those same tests are marked as "risky" saying that 4 assertions were run - even though the tests are empty.

This leads me to believe that somewhere assertions are being used within a fixture method (setUp() et al). This is bad practice and should not be allowed.

I've not dug in any deeper yet, but I do believe that this underlying issue needs to be solved first before this patch can be accepted.

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


3 months ago

#26 @johnbillion
3 months ago

Yes there are definitely non-test methods in the test suite that perform assertions. I'll try to dig them out.

#27 @jrf
3 months ago

@johnbillion Appreciated! Thanks.

#28 in reply to: ↑ 23 ; follow-up: @SergeyBiryukov
3 months ago

Replying to jrf:

Note: both methods used in the above two patches were introduced in PHPUnit 5.6.0. As the test runs on PHPUnit 5.x use PHPUnit 5.7.27, this shouldn't be an issue, though the test bootstrap.php file does give the impression that PHPUnit 5.4 is still supported (though I don't understand why we'd want to support anything but the last version of the PHPUnit 5.x series).

Right, WordPress test suite still supports PHPUnit 5.4.x as the minimum version as of [47880].

That said, I don't see any issue with bumping that requirement to PHPUnit 5.7.x.

#30 @jrf
3 months ago

WP_Test_REST_Controller_Testcase::filter_rest_url_for_leading_slash() would seem the most likely culprit.

The function is added as a callback to a filter in the (parent) WP_Test_REST_Controller_Testcase::setUp() and the rest_api_init action is also called during setUp() - I still have to trace whether that causes the filter_rest_url_for_leading_slash() method to run, but seems likely, though that one filter callback being called four times during setUp() does feel like there may be an underlying bug in the REST API Controller logic anyway.

While looking at this, I also noticed another issue with the setUp() and tearDown(): setUp() adds the filter_rest_url_for_leading_slash() method as a callback, but tearDown() tries to remove test_rest_url_for_leading_slash() as a callback.
Not a real problem as the WP_UnitTestCase_Base::tearDown() calls WP_UnitTestCase_Base::_restore_hooks() which reset the hooks queue to its original state, but just thought I'd mention it as it does seem off.

#31 @jrf
3 months ago

Confirmed: commenting out the assertion in the filter_rest_url_for_leading_slash() method removes the "This test is annotated with "@doesNotPerformAssertions" but performed 4 assertions" risky warning.

Still have to dig in deeper to figure out where the callback is triggered and what should be done with it.

#33 in reply to: ↑ 28 ; follow-up: @jrf
3 months ago

Replying to SergeyBiryukov:

Replying to jrf:
Right, WordPress test suite still supports PHPUnit 5.4.x as the minimum version as of [47880].

That said, I don't see any issue with bumping that requirement to PHPUnit 5.7.x.

@SergeyBiryukov FYI: The requirement will be raised to 5.7.21 in the polyfills patch anyway, so we could just wait a little for that patch to land before those two assertion change patches get committed ?

(assertions inside a data provider, nice)

@johnbillion Ouch... nice find though. I think we should aim to fix all of these in the foreseeable future.

#34 @SergeyBiryukov
3 months ago

In 51476:

Tests: Correct class name for WP_Filesystem_Base::find_folder() tests.

A concrete test class should be suffixed with Test, not UnitTestCase(s).

Follow-up to [25053].

Props jrf.
See #53363.

#35 @SergeyBiryukov
3 months ago

In 51478:

Tests: Add a $message parameter for custom assertions in WP_UnitTestCase_Base.

All assertions in PHPUnit have a $message parameter. Setting this parameter allows to distinguish which assertion is failing when a test runs multiple assertions, making debugging of the tests easier.

This optional parameter is now added for the assertion methods in the WP_UnitTestCase_Base class that were missing it.

Props jrf.
See #53363.

#36 @SergeyBiryukov
3 months ago

In 51479:

Tests: Correct placement of the $message parameter in assertDiscardWhitespace().

Follow-up to [51478].

Props johnbillion.
See #53363.

#37 @SergeyBiryukov
3 months ago

In 51480:

Tests: Modernize the WP_UnitTestCase_Base::assertEqualFields() method:

  • Use assertSame() instead of fail() to display a proper message in case of failure.
  • Add an optional $message parameter for consistency with other assertions.

Follow-up to [51478], [51479].

See #53363.

#38 @SergeyBiryukov
3 months ago

In 51481:

Tests: Use better assertions in WP_UnitTestCase_Base::assertEqualFields():

  • Check if the object attribute exists before checking its value.
  • Mention the field name in error messages in case of failure.

Follow-up to [51478], [51479], [51480].

Props jrf.
See #53363.

#39 @jrf
3 months ago

Found something else which needs looking into, where the tests clearly aren't self-contained:

If I add the import group to the <groups> <excludes>, then the following two unrelated tests start erroring out:

#) Tests_Embed_Template::test_oembed_output_post_with_thumbnail
DOMDocument::loadHTML(): AttValue: " expected in Entity, line: 10

path/to/wp/tests/phpunit/tests/oembed/template.php:66

#) Tests_Embed_Template::test_oembed_output_attachment
DOMDocument::loadHTML(): AttValue: " expected in Entity, line: 10

path/to/wp/tests/phpunit/tests/oembed/template.php:110

#40 follow-up: @johnbillion
3 months ago

@jrf Thanks, can you open a separate ticket for that please?

This ticket was mentioned in PR #1519 on WordPress/wordpress-develop by jrfnl.


3 months ago

  • Keywords has-patch has-unit-tests added

### QA: move incorrectly placed test file

The Block_Supported_Styles_Test class is not a TestCase to be extended, but an actual concrete test class.

Note: the tests as-is are failing. This should be fixed before this can be merged.

Includes minor CS fixes and adding the $message parameter to assertions to aid in debugging.

### Block_Supported_Styles_Test: update the style related test to current expected format

### Block_Supported_Styles_Test: remove two outdated tests

The functionality which these tests were testing is no longer available in this manner, so these tests are redundant.

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

#42 @jrf
3 months ago

I've just opened PR 1519 which is a collaborative effort between @aristath and me with some consulting from @youknowriad to fix a misplaced test.

The test was in the phpunit/includes directory, meaning it was never actually run.
When fixing the location though, turned out the tests were failing.
All fixed now and ready for review/commit.

#43 in reply to: ↑ 40 @jrf
3 months ago

Replying to johnbillion:

@jrf Thanks, can you open a separate ticket for that please?

Done: https://core.trac.wordpress.org/ticket/53781

@jrf
3 months ago

The three commits of GH PR 1519 as one patch ready for commit

#44 @SergeyBiryukov
3 months ago

In 51490:

Tests: Move and fix incorrectly placed tests for block supported styles.

The Block_Supported_Styles_Test class is not a TestCase to be extended, but an actual concrete test class. In order to run as expected, it should be placed under phpunit/tests/blocks/ along with the other block tests.

Additionally:

  • Add missing visibility keywords to test methods.
  • Update the expected results to the currently used format for the tests to pass.
  • Remove two outdated tests. The functionality being tested there is no longer available in this manner, so these tests are redundant.

Follow-up to [49226], [49310].

Props jrf, aristath, youknowriad.
See #53363.

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


3 months ago

#50 in reply to: ↑ 33 @jrf
2 months ago

Replying to jrf:

Replying to SergeyBiryukov:

Replying to jrf:
Right, WordPress test suite still supports PHPUnit 5.4.x as the minimum version as of [47880].

That said, I don't see any issue with bumping that requirement to PHPUnit 5.7.x.

@SergeyBiryukov FYI: The requirement will be raised to 5.7.21 in the polyfills patch anyway, so we could just wait a little for that patch to land before those two assertion change patches get committed ?

Both the 53363-02-use-assertfileisreadable.patch and the 53363-03-use-assertdirectoryexists.patch changes are now unblocked after the commits made for #47381, which raised the minimum PHPUnit version to 5.7.x as the assertion functions being introduced in those patches are available since PHPUnit 5.6.0.

@jrf
2 months ago

Build/Tests: fix message display in test bootstrap Any messages to the user which are echo-ed out in the test bootstrap will generally display on a command-line interface. The *nix specific "\n" line ending will be ignored on Windows, making the messages less readable. For new lines in CLI messages, PHP_EOL should be used instead. This was already done in a few places in the script, but not consistently so. Fixed now.

@jrf
2 months ago

PHPUnit config files: add schema reference The current config files validate against the PHPUnit XSD schema for config files for PHPUnit 5.7 - 9.2. The schema was changes in PHPUnit 9.3 and the filter and logging configuration deprecated in favour of coverage and a different format for logging. By setting the schema, deprecation notices about the use of an outdated schema when running on PHPUnit 9.3+ are prevented.

@jrf
2 months ago

Build/Tests: testcases should be abstract TestCases which are intended to be extended and not run directly, should be abstract.

#51 @SergeyBiryukov
2 months ago

In 51579:

Tests: Use more appropriate assertions in get_themes() tests.

This replaces instances of assertTrue( is_file( ... ) ) followed by assertTrue( is_readable( ... ) ) with assertFileIsReadable() to use native PHPUnit functionality.

The assertFileIsReadable() method was introduced in PHPUnit 5.6. As the minimum supported PHPUnit version has been raised to PHPUnit 5.7.21, it can now be used.

Follow-up to [51543], [51574].

Props jrf.
See #53363.

#52 @SergeyBiryukov
2 months ago

In 51580:

Tests: Use more appropriate assertions in get_themes() tests.

This replaces instances of assertTrue( is_dir( ... ) ) with assertDirectoryExists() to use native PHPUnit functionality.

The assertDirectoryExists() method was introduced in PHPUnit 5.6. As the minimum supported PHPUnit version has been raised to PHPUnit 5.7.21, it can now be used.

Follow-up to [51543], [51574], [51579].

Props jrf.
See #53363.

#53 @SergeyBiryukov
2 months ago

In 51581:

Build/Test Tools: Fix message display in test bootstrap.

Any messages to the user which are echo-ed out in the test bootstrap will generally display on a command-line interface.

The *nix specific "\n" line ending will be ignored on Windows, making the messages less readable.

For new lines in CLI messages, PHP_EOL should be used instead.

This was already done in a few places in the script, but not consistently so. Fixed now.

Follow-up to [UT882], [UT890], [44723], [45020], [48592], [49535], [51560].

Props jrf.
See #53363.

@jrf
2 months ago

Tests/bootstrap: fix unicode char having slipped in Follow up on [51581]

#54 @SergeyBiryukov
2 months ago

Just noting that I was confused by 53363-07-add-schema-to-config.patch at first, as I still see a deprecated schema warning in PHPUnit 9.3+ with this patch, both locally and on GitHub:

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

It looks like either this message or a more detailed one:

Warning - The configuration file did not pass validation!
The following problems have been detected:
...
Test results may not be as expected.

is displayed in TestRunner::run(), depending on whether a schema is detected.

Per discussion with @jrf, the first warning will remain and is fine. Only when PHPUnit 10 comes into play we should look into adding a run step to CI to call the --migrate-configuration command. The patch is supposed to silence the second, more detailed warning, displayed in PHPUnit 9.3+ when an old configuration file is used.

Looking at the SchemaDetector class, it attempts to validate the configuration file against the available schema files, as per the SchemaFinder class.

In my testing with PHPUnit 9.5.8, the configuration file successfully validates against the PHPUnit 9.2 schema with or without it being explicitly declared, so the detailed message is never displayed and the patch does not seem to make a difference. Still, I guess it's better to declare the schema explicitly for clarity.

#55 @SergeyBiryukov
2 months ago

In 51583:

Build/Test Tools: Add schema reference to PHPUnit config files.

The current config files validate against the PHPUnit XSD schema for config files for PHPUnit 5.7 – 9.2.

The schema was changed in PHPUnit 9.3, and the filter and logging settings were deprecated in favor of coverage and a different format for logging.

This commit explicitly sets the schema against which the files currently validate, for clarity.

Props jrf.
See #53363.

#56 @SergeyBiryukov
2 months ago

In 51584:

Build/Test Tools: Remove Unicode character from PHPUnit version check message.

Not all CLI tools can handle Unicode characters or non-system specific line endings well, so this type of CLI messaging should always be written with the optimal cross-platform, cross-CLI tool end-user experience in mind.

Follow-up to [51581].

Props jrf.
See #53363.

#57 @SergeyBiryukov
2 months ago

In 51585:

Build/Test Tools: Declare two TestCase classes as abstract.

TestCases which are intended to be extended and not run directly, should be abstract.

Follow-up to [763/tests], [30277].

Props jrf.
See #53363.

#59 @SergeyBiryukov
2 months ago

In 51628:

Tests: Move loading the PO class to set_up_before_class().

This ensures that the class is loaded once before the first test of the test case class is run, and require_once() is not unnecessarily called for each test method individually.

Follow-up to [1106/tests].

See #53363.

#61 @SergeyBiryukov
2 months ago

In 51641:

Tests: Move loading the WP_Community_Events class to set_up_before_class().

This ensures that the class is loaded once before the first test of the test case class is run, and require_once() is not unnecessarily called for each test method individually.

Follow-up to [40607], [51628].

See #53363.

#65 @SergeyBiryukov
7 weeks ago

In 51663:

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

The tests were partially duplicated in two separate files.

Follow-up to [431/tests], [28900], [38928], [42527].

See #53363, #53987.

#66 @SergeyBiryukov
7 weeks ago

In 51664:

Tests: Remove duplicate wp_list_pluck() tests.

The tests were partially duplicated in two separate files, and are now located in their own file.

Follow-up to [431/tests], [28900], [38928], [42527], [51663].

See #53363, #53987.

#67 @SergeyBiryukov
7 weeks ago

In 51665:

Tests: Rename the test file and class for wp_filter_object_list() tests.

This matches the name of the function being tested.

Follow-up to [410/tests], [51663], [51664].

See #53363, #53987.

@jrf
7 weeks ago

Tests bootstrap: improve an outdated error condition The PHPUnit tests are/should generally be run on the src directory, so changes just made can be tested. While testing via the build directory is also still supported, this is not the default. This was last changed in [50441], but that commit did not remove/update the error message thrown by the test bootstrap file. This last change also did not take into account that that change meant that people had to update their own wptests-config.php file to match the change made in Core and would otherwise still get the outdated error message. This commit changes the messaging in the test bootstrap file to be more in line with the current reality, while still accounting for the fact that tests can be run from both src as well as build`.

#68 @SergeyBiryukov
7 weeks ago

In 51666:

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

This matches the name of the function being tested.

Follow-up to [38928], [51663-51665].

See #53363, #53987.

#69 @SergeyBiryukov
7 weeks ago

In 51667:

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

This matches the name of the function being tested.

Follow-up to [38928], [51663-51666].

See #53363, #53987.

#70 @SergeyBiryukov
7 weeks ago

In 51668:

Coding Standards: Add missing visibility keywords for wp_filter_object_list() and wp_list_pluck() tests.

Follow-up to [51663-51667].

Props pbearne.
See #53363, #53987.

#71 @hellofromTonya
7 weeks ago

@jrf and I discussed 53363-09-test-bootstrap-improve-error-msg.patch per notes she added to the patch attachment. I tested before and after with each scenario. Works as expected.

Test Report

Env:

  • WordPress: trunk
  • OS: macOS Big Sur
  • localhost: npm / Docker

Setup:

npm install
npm run build:dev
npm run env:start
npm run env:install
composer install

Scenario 1: ABSPATH is build but build folder does not exist

Steps:

  • Change the ABSPATH to build in the wp-tests-config.php file:
    define( 'ABSPATH', dirname( __FILE__ ) . '/build/' );
    
  • Run the tests (here running one group of tests but could be any group or all tests)
    npm run test:php -- --group formatting
    

Results:
Before patch:

Error: The /build/ directory is missing! Please run `npm run build` prior to running PHPUnit.

After applying patch:

Error: The PHPUnit tests should be run on the /src/ directory, not the /build/ directory. Please update the ABSPATH constant in your `wp-tests-config.php` file to `dirname( __FILE__ ) . '/src/'` or run `npm run build` prior to running PHPUnit.

Works as expected ✅

Scenario 2: ABSPATH is not src or build and the folder does not exist

Steps:

  • Change the ABSPATH to doesnotexist in the wp-tests-config.php file:
    define( 'ABSPATH', dirname( __FILE__ ) . '/doesnotexist/' );
    
  • Run the tests (here running one group of tests but could be any group or all tests)
    npm run test:php -- --group formatting
    

Results:
Before patch:

Error: The /build/ directory is missing! Please run `npm run build` prior to running PHPUnit.

After applying patch:

Error: The ABSPATH constant in the `wp-tests-config.php` file is set to a non-existent path "/var/www/doesnotexist/". Please verify.

Works as expected ✅

Scenario 3: ABSPATH is build and build folder exists

Steps:

  • In command-line, run: npm run build
  • Change the ABSPATH to build in the wp-tests-config.php file:
    define( 'ABSPATH', dirname( __FILE__ ) . '/build/' );
    
  • Run the tests (here running one group of tests but could be any group or all tests)
    npm run test:php -- --group formatting
    

Results:
No errors. Tests run. Works as expected ✅

#72 @hellofromTonya
7 weeks ago

Whoopsie, I forgot to add the ticket number to changeset [51669]. Commenting here to (manually) link the changeset to this ticket.

In 51669:
Tests: Improve bootstrap error message for when ABSPATH folder does not exist.

The PHPUnit tests are/should generally be run on the src directory, so changes just made can be tested. While testing via the build directory is also still supported, this is not the default.

This was last changed in [50441], but that commit did not remove/update the error message thrown by the test bootstrap file.

This last change also did not take into account that that change meant that people had to update their own wptests-config.php` file to match the change made in Core and would otherwise still get the outdated error message.

This commit changes the messaging in the test bootstrap file to be more in line with the current reality, while still accounting for the fact that tests can be run from both src as well as build.

Follow-up to [49569], [50441], [51581].

Props jrf, hellofromTonya.

Last edited 7 weeks ago by hellofromTonya (previous) (diff)

#73 @desrosj
7 weeks ago

In 51670:

Build/Test Tools: Remove shell: bash from PHPUnit test workflow.

bash is the default shell for all non-Windows GitHub Action runners, so this is not necessary.

See #53363.

#74 @desrosj
7 weeks ago

In 51671:

Build/Test Tools: Remove shell: bash from code coverage workflow.

bash is the default shell for all non-Windows GitHub Action runners, so this is not necessary.

Follow up to [51670].
See #53363.

@jrf
7 weeks ago

WP_UnitTestCase_Base: more improvements to custom assertions The assertEqualFields() method expects an object and a fields array as inputs and subsequently approaches the received parameters as such, but did not verify whether the received parameters are of the expected types. Along the same lines, the assertSameSets(), assertEqualSets(), assertSameSetsWithIndex() and the assertEqualSetsWithIndex() methods all expect arrays for both the actual as well as the expected values and uses the array function [k]sort() on both, but never verified that the received inputs were actually arrays, which could lead to PHP errors on the sorting function calls.

@jrf
7 weeks ago

WP_UnitTestCase_Base::assertDiscardWhitespace(): stabilize the custom assertion The assertDiscardWhitespace() method uses assertEquals() under the hood, meaning that in reality any type of actual/expected value should be accepted by the function. Fixed the documentation to reflect that. At the same time, only strings can contain whitespace differences, so the whitespace replacement should only be done when string values are passed. This will prevent potential passing null to non-nullable errors on PHP 8.1, if either of the inputs would turn out to be null.

#75 @jrf
7 weeks ago

Uploaded two more patches for the custom assertions. Both are ready for review.

#76 @hellofromTonya
7 weeks ago

In 51697:

Tests: Test custom assertions parameter data type in WP_UnitTestCase_Base.

The following changes improve tests stability.

The assertEqualFields() method expects an object and a fields array as inputs and subsequently approaches the received parameters as such, but did not verify whether the received parameters are of the expected types.

Along the same lines, the assertSameSets(), assertEqualSets(), assertSameSetsWithIndex() and the assertEqualSetsWithIndex() methods all expect arrays for both the actual as well as the expected values and uses the array function [k]sort() on both, but never verified that the received inputs were actually arrays, which could lead to PHP errors on the sorting function calls.

Follow-up to [30687], [42343], [48937], [48939], [51480], [51481].

Props jrf.
See #53363.

#77 @hellofromTonya
7 weeks ago

In 51698:

Tests: Do whitespace replacement in assertDiscardWhitespace() only when string.

The assertDiscardWhitespace() method uses assertEquals() under the hood, meaning that in reality any type of actual/expected value should be accepted by the function. Fixed the documentation to reflect that.

At the same time, only strings can contain whitespace differences. So the whitespace replacement should only be done when string values are passed.

This change (a) prevents potential passing null to non-nullable errors on PHP 8.1, if either of the inputs would turn out to be null and (b) increases tests stability.

Follow-up to [35003], [44902].

Props jrf.
See #53363.

#78 @hellofromTonya
5 weeks ago

In 51792:

Tests: Add more invalid IP test cases and @covers to Tests_Functions_Anonymization.

Adds a few more invalid IP test cases.

Adds extra @covers tag for the two functions which are testing the wp_privacy_anonymize_ip() function.

(At class level, the wp_privacy_anonymize_data() is set as covered).

Follow-up to [42971].

Props jrf, hellofromTonya.
See #53363.

#79 @hellofromTonya
5 weeks ago

In 51798:

Tests: Add tests for wpdb::_real_escape().

Adds a new dedicated test file.

Adds a test to check that various input types passed to wpdb::_real_escape() are handled correctly.

Note: This new test does not test the actual escaping or other logic in the function. Rather, it just and only tests and documents how the function handles various input types.

Props jrf, hellofromTonya.
See #53363.

#80 @hellofromTonya
4 weeks ago

Whoopsie attached changeset [51817] to the wrong ticket. It belongs on this ticket.

In 51817:
Build/Test Tools: Reworks Tests_Option_Option::test_bad_option_names() into a data provider.

The existing tests were running multiple functions through a foreach(). If any test failed, it would bail out and not test against the other scenarios.

This commit:

  • Moves the scenarios to a data provider with named data sets, i.e. to ensure all scenarios are run and tested regardless if any fail.
  • Splits each function under test into individual test methods.
  • Adds a float scenario.
  • Adds method visibility modifiers.

Follow-up to [25002].

Props jrf, hellofromTonya, pbearne.
See #53635.

#81 @hellofromTonya
4 weeks ago

In 51831:

Build/Test Tools: Fix null handling and string type casting in WP_UnitTestCase_Base::assertSameIgnoreEOL().

Basically, the whole assertSameIgnoreEOL() assertion was fundamentally flawed. The assertion contends that it checks that the expected and actual values are of the same type and value, but the reality was very different.

  • The function uses map_deep() to potentially handle all sorts of inputs.
  • map_deep() handles arrays and objects with special casing, but will call the callback on everything else without further distinction.
  • The callback used passes the expected/actual value on to the str_replace() function to remove potential new line differences.
  • And the str_replace() function will - with a non-array input for the $subject - always return a string.
  • The output of these calls to map_deep() will therefore have "normalized" _all properties_ in objects, _all values_ in arrays and _all non-object, non-array values_ to strings.
  • And a call to assertSame() will therefore NEVER do a proper type check as the type of all input has already, unintentionally, been "normalized" to string.

Aside from this clear flaw in the design of the assertion, PHP 8.1 now exposes a further issue as a null value for an object property, an array value or a plain value, will now yield a str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated notice.

To fix both these issues, the fix in this PR ensures that the call to str_replace() will now only be made if the input is a text string.
All other values passed to the callback are left in their original type.

This ensures that a proper value AND type comparison can be done as well as prevents the PHP 8.1 deprecation notices.

Ref:

This commit:

  • Fixes type-casting of non-string values to string (the flawed part of this assertion) by invoking str_replace() when the value is of string type.
  • Fixes the PHP 8.1 str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated deprecation notice.
  • Micro-optimization: skips map_deep() when actual and/or expected are null (no need to process).
  • Adjusts the method documentation for both this method and the assertEqualsIgnoreEOL() alias method to document that the $expected and $actual parameters can be of any type.

Follow-up to [48937], [51135], [51478].

Props jrf, hellofromTonya.
See #53363, #53635.

#82 @hellofromTonya
3 weeks ago

In 51852:

Build/Test Tools: Splits and improves compat tests.

Splits the tests in the tests/phpunit/tests/compat.php file up into individual test classes for each function being tested.

Improvements to individual test cases:

  • Adds @covers tags.
  • Adds visibility modifiers to all methods.
  • Adds function availability test.
  • Where relevant, fixes the assertion parameter order.
  • Data provider:
    • Where relevant, reworks a test to use a data provider.
    • Where relevant, renames data provider methods to have a more obvious link to the test it applies to.
    • Makes the data provider more readable by adding keys within the data sets.
    • Moves the data provider below its associated tests.
    • Adds/removes data sets in data providers.
  • Makes the actual test code more readable by using descriptive variables and multi-line function calls.
  • Adds the $message parameter to all assertions when a test method contains more than one assertion.

Specifically for the _mb_substr() tests:

  • Splits the test_mb_substr_phpcore() method into two test methods based on the PHP Core test files they are emulating.
  • Makes the actual test code within the test_mb_substr_phpcore_basic() method more readable by using descriptive variables and multi-line function calls.
  • Splits the data used for the second part of the test_mb_substr_phpcore() function, now test_mb_substr_phpcore_input_type_handling(), off into a separate data provider with named data sets.
  • Removes duplicate data sets from the data_mb_substr_phpcore_input_type_handling().
    • Why? The PHP native tests test against upper/lowercase false, true, null and some other text string single quote/double quote variations. As things were, those differentiations had been undone when the coding standards were put in place, so in effect those weren't being tested anymore. And as this is userland code, there's no point in adding these differentiations back as they will be handled the same by PHP anyway (and that is safeguarded via the PHP native tests).
  • Removes the "undefined variable" and "unset variable" test cases as, while those are relevant to the C code in which PHP is written, they are not relevant for testing userland code and will behave the same as the test passing null.

Follow-to [25002], [32364], [42228], [42343], [43034], [43036], [43220], [43571], [45607], [47122], [47198], [48937], [48996], [51415], [51563], [51594].

Props jrf, hellofromTonya.
See #39265, #53363.

#83 @SergeyBiryukov
3 weeks ago

In 51858:

Tests: Correct the @ticket reference in wp_terms_checklist() tests.

Follow-up to [48880].

See #53363, #51137.

#85 @SergeyBiryukov
3 weeks ago

In 51869:

Tests: Remove unnecessary setUp() and tearDown() methods in multisite tests.

These were originally added in [26252] to suppress database errors on setUp() and restore on tearDown() for tests that call wpmu_create_blog(), blog factory, or installation code that attempts to clear transients.

As the multisite test coverage expanded, these methods ended up being unnecessarily copied into other test classes, where database error suppression is not required.

Follow-up to [26252], [29916], [30286], [33184], [34898], [34899], [34901], [37234], [37477], [37894], [49212], [49616], [51859].

See #53363.

This ticket was mentioned in PR #1709 on WordPress/wordpress-develop by ntwb.


3 weeks ago

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

Whilst running PHPUnit locally I noticed this message from PHPUnit 9.3:

Warning: Your XML configuration validates against a deprecated schema.
Suggestion: Migrate your XML configuration using "--migrate-configuration"!

I broke down the commits in this PR to aid code-review, one commit for each PHPUnit XML file, and additional commits for white-space cleanup.

This ticket was mentioned in Slack in #core-test by netweb. View the logs.


3 weeks ago

#88 @SergeyBiryukov
3 weeks ago

In 51870:

Tests: Don't skip some Ajax tests on multisite, add them to the ms-excluded group instead.

Follow-up to [46693], [49835].

See #53363.

#89 @prbot
3 weeks ago

ntwb commented on PR #1709:

Closing per comment https://core.trac.wordpress.org/ticket/54183#comment:13

Note: I would recommend against the config file update for the PHPUnit 9.3+ schema for now (PR 1709).
The old config will work fine for the time being and works cross-version, while the PHPUnit 9.3+ config would only work for PHPUnit 9.3+.

By the time PHPUnit 10 comes out (and the old config is no longer supported in PHPUnit 10), we can start to use the "on the fly" migrate option in CI if needs be:

phpunit --migrate-configuration

#90 @SergeyBiryukov
3 weeks ago

In 51872:

Tests: Update the Services_JSON test for PHPUnit 9.5.10/8.5.21+.

Since PHPUnit 9.5.10 and 8.5.21, PHP deprecations are no longer converted to exceptions by default (convertDeprecationsToExceptions="true" can be configured to enable this).

Reference: Do not convert PHP deprecations to exceptions by default; PHPUnit 9.5.10 changelog.

With this change, the test for the Services_JSON compat class started failing:

There was 1 failure:

1) Tests_Compat_jsonEncodeDecode::test_json_encode_decode
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

This converts the native PHPUnit ::expectDeprecation() method call in the test to a set of individual WP-specific ::setExpectedDeprecated() method calls in order to not depend on PHPUnit behavior that is no longer the default.

Additionally, this commit includes support for catching deprecation notices from _deprecated_file() function calls to the WP_UnitTestCase_Base::expectDeprecated() method.

Follow-up to [46205], [46625], [48996], [51563], [51852], [51871].

Props jrf, netweb, SergeyBiryukov.
See #54183, #54029, #53363.

Note: See TracTickets for help on using tickets.