WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 14 hours 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:
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 (5)

53363-01.patch (8.4 KB) - added by jrf 5 days 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 5 days 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 5 days 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 5 days 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 5 days 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.

Download all attachments as: .zip

Change History (44)

#1 @SergeyBiryukov
3 weeks 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 weeks 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 weeks 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 weeks 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 weeks 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
2 weeks 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
2 weeks 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
2 weeks 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
2 weeks 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.


11 days ago

#11 @SergeyBiryukov
11 days 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
11 days 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
10 days 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
9 days 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
9 days 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
9 days 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
8 days 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
8 days 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
8 days 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
7 days 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
7 days 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
5 days ago

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

@jrf
5 days ago

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

@jrf
5 days ago

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

#23 follow-up: @jrf
5 days 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
5 days ago

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

@jrf
5 days 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
5 days 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.


4 days ago

#26 @johnbillion
4 days ago

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

#27 @jrf
4 days ago

@johnbillion Appreciated! Thanks.

#28 in reply to: ↑ 23 ; follow-up: @SergeyBiryukov
4 days 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
4 days 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
4 days 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 @jrf
4 days 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
4 days 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
4 days 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
4 days ago

In 51479:

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

Follow-up to [51478].

Props johnbillion.
See #53363.

#37 @SergeyBiryukov
4 days 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 days 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
14 hours 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
Note: See TracTickets for help on using tickets.