Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#53911 closed task (blessed) (fixed)

Test modernization backwards-compatibility backports to help security test backports and extenders with WordPress cross-version testing

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

Description (last modified by hellofromTonya)

Related to: #46149, #47381, #53904

Goals:

  • Allow tests which need to be backported for the security test suite to be backported to older WP versions without the need to make significant changes to these tests to get them to run.
  • Make it easier for extenders who are using core's test case(s) to be able to have one test suite and still be able to run it against multiple WP versions, including WP 5.9.
  • Not make significant changes to the test suites for older WP versions including not making older WP versions run PHPUnit 8+.

The scope of work was discussed during a live working session with @jrf, @johnbillion, @sergeybiryukov, and me. Listen or watch this live session here and here.

Scope includes:

  • Determine how far back to backport
  • Determine what test modernization changes to backport
  • Add wrappers for the new snake_case fixture methods

Determine how far back to backport

The PHPUnit Polyfills includes traits and namespaces and supports a minimum of PHP 5.4. WordPress had a minimum PHP 5.2 support until WordPress 5.2, which increased it to PHP 5.6.

As part of the discovery process, this ticket will also include a list of popular plugins and their minimum supported version.

Add wrappers for new snake case fixture methods

5.9-alpha and newer use snake_case fixture methods (see changeset [51568] for more information). Extender's tests using the native PHPUnit fixture test methods will need to be changed to the new snake_case fixture methods.

What about WordPress cross-version testing? In <= WordPress 5.8, the snake_case fixture methods did not exist.

This ticket seeks to add wrappers for WordPress versions before 5.9 to help extenders with their testing suites.

Change History (78)

#1 @hellofromTonya
3 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Owner set to johnbillion
  • Status changed from new to assigned
  • Type changed from defect (bug) to task (blessed)

#2 @hellofromTonya
3 years ago

The following is a matrix of the popular plugins.

Plugins with >= PHP 5.4 minimum:

Plugin min PHP version min WP version has PHPUnit test?
Advanced Custom Fields 5.6 4.7 No
Advanced Editor Tools 5.6 5.6 ?
Contact Form 7 5.6 5.5 No
Contact Form by WPForms 5.5 4.9 ?
Elementor 5.6 5.0 Yes
Jetpack 5.6 5.7 Yes
Really Simple SSL 5.6 4.9 No
Redirection 5.6 5.2 ?
WooCommerce 7.0 5.5 Yes
WordPress Importer 5.6 5.2 Yes
WP Mail SMTP 5.6.20 4.9 ?
Yoast Duplicate Post 5.6.20 5.6 Yes
Yoast SEO 5.6.20 5.6 Yes

Plugins with < PHP 5.4 minimum:

Plugin min PHP version min WP version has PHPUnit test?
Akismet 5.2 4.6 ?
All-in-One WP Migration 5.2.17 3.3 ?
MC4WP: Mailchimp for WordPress 5.3 4.6 No
MonsterInsights 5.2 3.8 ?
Wordfence Security 5.3 3.9 ?
WP Super Cache 5.2.4 3.1 No
XML Sitemaps 5.2 3.3 ?

Plugins with unknown PHP minimum:

Plugin min PHP version min WP version has PHPUnit test?
Duplicate Page ? 3.4 ?
LiteSpeed Cache ? 4.0 No
UpdraftPlus WordPress Backup ? 3.2 ?
Last edited 3 years ago by hellofromTonya (previous) (diff)

#3 @jrf
3 years ago

I've done a careful assessment of what is feasible and necessary for the backports and @hellofromTonya and me have discussed this.

As things are, we'd like to propose the following for the backports:

WordPress 5.2 - 5.8

  • Add PHPUnit Polyfills as a Composer dependency.
  • Add the Polyfill traits to the WP_UnitTestCase.
  • Add the loading of the Polyfill autoloader to the bootstrap in the same way as done for WP 5.9.
  • Add set_up(), tear_down() etc wrappers to the WP_UnitTestCase class. These fall through to the camelCase methods in the WP_UnitTestCase_Base class.
  • Add set_up(), tear_down() etc wrappers to the other WP native TestCases within the tests/phpunit/includes directory.

WordPress 5.1 and lower

  • Backport just and only the fixture method wrappers and nothing else.

It is not an option to introduce the PHPUnit Polyfills to older WP versions as the minimum PHP requirement for the Polyfills in PHP 5.4, due to the use of traits.

Integration test suites for plugins/themes which still support older WP versions, but have a minimum PHP requirement of PHP 5.4, can still introduce the Polyfills in their own test suite for those older WP versions.

At this point it is undecided how far back to backport the fixture method wrappers.

Impact Assessment

➕ Allows for (security) tests backported for WP Core itself to work without changes using the features as per WP 5.9 for WP 5.2 to 5.8. For older WP versions, some assertion/expectation method calls may need to be switched out for their "old" name.
➕ Allows integration tests to switch completely to snake_case fixture method names.
➕ Allows integration tests to use PHPUnit 9.x assertion and expectation methods via the Polyfills for WP 5.2 and higher. When the test suite would also need to run in combination with older WP versions, the Polyfills can still be used, as long as the tests are run on PHP 5.4 or higher.
➕ It prevents breaking integration test suites which haven't updated to the "WP 5.9 way" yet.

➖ Still limits running of the tests on WP < 5.9 to PHPUnit 5.x - 7.x.
➖ With as a consequence that integration tests will need to do some PHPUnit version juggling, probably via Composer, in their CI.

#4 @hellofromTonya
3 years ago

WordPress 5.1 and lower
Backport just and only the fixture method wrappers and nothing else.

Backporting the fixture method wrappers means that extenders can (a) make a one-time change to snake_case and then (b) continue testing against older versions of WordPress.

Looking at the popular plugins matrix, there are plugins that support versions older than WP 5.2. If any of these plugins extend from core's test case(s), then backporting past WP 5.2 helps them and provides a better dev experience.

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


3 years ago
#5

  • Keywords has-patch has-unit-tests added

Implementation of the backport proposal for WP 5.2 - 5.8 and lower.

## Commit Details

### Build/Test Tools: Add Composer dependency on the PHPUnit Polyfills package.

The PHPUnit Polyfills package is an add-on for PHPUnit, which works around common issues for writing PHPUnit cross-version compatible tests.

Features:

  • It offers a full set of polyfills for assertions and expectations introduced in PHPUnit since PHPUnit 4.8.
  • It offers two generic TestCases which include these polyfills, but also solve the void return type issue for the fixtures methods.
  • It offers a PHPUnit cross-version solution for the changes to the PHPUnit TestListener implementation.
  • Supports PHPUnit 4.8 – current.
  • Supports and is compatible with PHP 5.4 – current.

The package has no outside dependencies, other than PHPUnit, is actively maintained and endorsed by the maintainer of PHPUnit itself (the only package of its kind which has ever been endorsed).

While use of the package will not be actively implemented in WP < 5.9, adding the dependency will allow for future backports of (security) tests to WP 5.2 - 5.8 to be executed without the need for adjusting the tests.

The minimum supported PHPUnit version, as checked in the test bootstrap file, has been adjusted to comply with the minimum version supported by the PHPUnit Polyfills.

### Build: always run composer install for the test workflows

As the PHPUnit Polyfills is now a requirement, the test workflows need to run some form of composer install to make them available.
The tests themselves can, and will, still be run via a PHPUnit PHAR file.

Note: The version juggling done to get Composer to install on all PHP versions, means that in select cases, the composer.lock file and sometimes even the composer.json file needs to be changed on the fly.
This would then fail then build on the "Ensure version-controlled files are not modified" step.
To mitigate this, a very select git checkout -- file... command is added right after the Composer install to reset those files to their committed state.
This way any _real_ changes to version controlled files will still be picked up on, but the build won't fail on these changes made specifically for the Composer install.

Second note: alternatively, a choice could be made to forego the composer install in favour of cloning the Polyfills repo in a vendor/yoast/phpunit-polyfills directory for CI.
That should also allow for the tests to be able to run and would prevent making changes to the composer.json/composer.lock files.

### Tests: make the polyfills available to all tests

... and add loading of the polyfills to the test bootstrap with the same messages as per PR 1587.

### Tests: introduce wrapper method for the PHPUnit fixture methods

The wrapper methods are intended to make it easier for plugin/theme integration tests to make their test suite compatible with the changes in WordPress 5.9, while still allowing the test suite to be run on WP < 5.2.

Plugin/theme integration test suites which still want to run their tests against WP < 5.2 will need to handle conditionally making the assertion and expectation polyfill traits available based on the WP version on which the tests are being run from within their own test suite, but with these wrapper methods in place, the fixture method rename will no longer be problematic.

Notes:
As these wrapper methods use static::..., these wrapper methods will automatically call the camelCase method name in the "highest" class in the hierarchy, effectively making the wrapper methods available to all test case classes.

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

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


3 years ago
#6

Implementation of the backport proposal for WP 5.1 and lower.

## Commit Details

### Tests: introduce wrapper method for the PHPUnit fixture methods

The wrapper methods are intended to make it easier for plugin/theme integration tests to make their test suite compatible with the changes in WordPress 5.9, while still allowing the test suite to be run on WP < 5.2.

Plugin/theme integration test suites which still want to run their tests against WP < 5.2 will need to handle conditionally making the assertion and expectation polyfill traits available based on the WP version on which the tests are being run from within their own test suite, but with these wrapper methods in place, the fixture method rename will no longer be problematic.

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

#7 @jrf
3 years ago

  • Keywords has-unit-tests removed

I've opened a PR against WP 5.8 with the backport implementation for WP 5.2 - 5.8 and a second PR against WP 5.1 with the backport implementation for WP 5.1 and lower.

Reviews and feedback welcome.

hellofromtonya commented on PR #1588:


3 years ago
#8

Waiting for PR #1587 to be merged before doing a final review of this PR. Why? Changes in that PR will affect this one.

jrfnl commented on PR #1588:


3 years ago
#9

@hellofromtonya FYI: the bootstrap messaging in this PR is already aligned with the bootstrap changes proposed in #1587, but yes, if #1587 would require further changes, this PR needs an update too.

#10 follow-up: @jorbin
3 years ago

The only version of WordPress that is supported is the latest, which is currently 5.8. Backporting before then should be off the table unless we want to change that policy. Backporting to help plugins continue to support older versions only encourages individuals to not update WordPress to a supported version.

#11 in reply to: ↑ 10 @swissspidy
3 years ago

Replying to jorbin:

The only version of WordPress that is supported is the latest, which is currently 5.8. Backporting before then should be off the table unless we want to change that policy. Backporting to help plugins continue to support older versions only encourages individuals to not update WordPress to a supported version.

I get that, but no plugin really only supports the latest version of WordPress and nothing older. The reality is that most plugins support something like WP 5.3+ onwards (or more). Without these tests-only backports, it becomes extremely difficult for them to run tests against WordPress core.

IMO, backporting these tests-only changes does not affect the policy of saying only the latest version of WordPress is officially supported, but it makes lives easier for thousands of developers extending WordPress through plugins and themes.

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


3 years ago

#13 follow-ups: @azaozz
3 years ago

  • Keywords 2nd-opinion added

Don't think this is ready for backporting too. The reason is that the main task of updating the WP tests to run on the latest PHPUnit version is not complete yet.

Two of the "top" requirements haven't been implemented yet: to actually be able to run the WP tests in PHPUnit 9.5, and to be able to install the polyfills (actually they are shims) globally, not in the checkout dir (that would support installing them in the Docker image, see #53945).

So, unless I'm missing something, what's being backported here seems to be "half-done" changes.

Last edited 3 years ago by azaozz (previous) (diff)

#14 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
3 years ago

Replying to azaozz:

Two of the "top" requirements haven't been implemented yet: to actually be able to run the WP tests in PHPUnit 9.5

WordPress tests on PHP 7.3 or later do currently run on PHPUnit 9.5.8 as of [51574]:
https://github.com/WordPress/wordpress-develop/runs/3376106790?check_suite_focus=true#step:20:27

#15 in reply to: ↑ 14 @azaozz
3 years ago

Replying to SergeyBiryukov:

Right, but to be able to run them you still have to install all of the polyfills/shims and everything else there (which doesn't make much sense). Normally they will be needed only when running on previous versions of PHPUnit, i.e. be "true" polyfills :)

Last edited 3 years ago by azaozz (previous) (diff)

#16 in reply to: ↑ 13 ; follow-up: @jrf
3 years ago

Replying to azaozz:

Two of the "top" requirements haven't been implemented yet: to actually be able to run the WP tests in PHPUnit 9.5, and to be able to install the polyfills (actually they are shims) globally, not in the checkout dir (that would support installing them in the Docker image, see #53945).

I have a feeling there is a misunderstanding here.

The backports are not intended to make significant changes to the test suites for older WP versions.
They are intended to:

  1. Allow tests which need to be backported for the security test suite to be backported to older WP versions without the need to make significant changes to these tests to get them to run.
  2. Make it easier for integrators to be able to have one test suite and still be able to run it against multiple WP versions, including WP 5.9.

These backports are *not* intended to make older WP test suites compatible with higher PHPUnit versions then they previously supported, nor to make it possible for older WP versions to run against higher PHP versions then they previously supported.

In other words:

  1. The tests for WP 5.9 are running on PHPUnit 9.5.
  2. Changing the supported PHPUnit version(s) for older WP versions is not on the table (and would require significantly more changes).

As for the ability to install the Polyfills (or shims if you prefer to call them that) globally, that's already included in the current backport proposal. The logic needed for that is in the test bootstrap.

#17 @hellofromTonya
3 years ago

  • Description modified (diff)
  • Summary changed from Test modernization backwards-compatibility backports to help extenders with WordPress cross-version testing to Test modernization backwards-compatibility backports to help security test backports and extenders with WordPress cross-version testing

Updated to include ticket goals including making it easier to backport tests for security backports.

#18 in reply to: ↑ 16 ; follow-ups: @azaozz
3 years ago

Replying to jrf:

These backports are *not* intended to make older WP test suites compatible with higher PHPUnit versions then they previously supported, nor to make it possible for older WP versions to run against higher PHP versions then they previously supported.

Great, thanks for the explanation. Guessing that if there's a way to not load the polyfills when using latest PHPUnit, that will be backported to WP 5.2 - 5.8 too.

#19 in reply to: ↑ 18 @SergeyBiryukov
3 years ago

Replying to azaozz:

Great, thanks for the explanation. Guessing that if there's a way to not load the polyfills when using latest PHPUnit, that will be backported to WP 5.2 - 5.8 too.

The Polyfills are not only used to polyfill the latest PHPUnit functionality on older versions, but also as a solution to the void return type declaration added to fixture methods in PHPUnit 8.0, see [51568] and #46149 ticket description.

#20 in reply to: ↑ 18 @jrf
3 years ago

Replying to azaozz:

Great, thanks for the explanation. Guessing that if there's a way to not load the polyfills when using latest PHPUnit, that will be backported to WP 5.2 - 5.8 too.

As this ticket is about the backports, the polyfills are needed as the tests on WP < 5.9 will never run on the latest PHPUnit, but always on older, outdated PHPUnit versions.

The polyfills being available means, that those security tests I mentioned earlier, can still be backported to older WP versions without needing to adapt them to those older, outdated PHPUnit versions, even when they use PHPUnit 9.x assertions/expectation methods.

As for WP 5.9 - which is outside the scope of this ticket -, as @SergeyBiryukov correctly points out, the polyfills are also used when running the latest PHPUnit version (9.5.x) as they solve the void conundrum via the snake_case fixture methods.

The functionality from the polyfills is lightweight and only those polyfills which are actually needed on any particular PHPUnit version are loaded. When the functionality is not needed, an empty trait is loaded and the assertions/expectations will just fall through to the PHPUnit native functionality.

I invite you to have a good look at the code in the Polyfill repo, including the autoload mechanism and you will see what I mean.

#21 @azaozz
3 years ago

but also as a solution to the void return type declaration added to fixture methods in PHPUnit 8.0

the polyfills are also used when running the latest PHPUnit version (9.5.x) as they solve the void conundrum via the snake_case fixture methods

Yep, I understand, and yes, not the right place to discuss, sorry :)

#22 @azaozz
3 years ago

  • Keywords 2nd-opinion removed

Thinking this is ready to go in.

The only version of WordPress that is supported is the latest, which is currently 5.8. Backporting before then should be off the table...

Right, that's for new bugfixes. Regressions and major bugfixes are fixed in trunk and backported to the latest branch (for dot releases), and security fixes/updates are backported to as many branches as feasible/possible.

Major changes to the build/test tools may sometimes need to be backported to older branches too as that would affect building and/or testing future dot releases. Agree this should be considered an exception rather than a rule. As the build/test tools are not "production code", thinking such exceptions are warranted when they would make core development a bit simpler/easier in the future.

If this change is considered together with the changes in #53945, it will become very easy for everybody to run the WP tests for the latest plus several older branches.

Last edited 3 years ago by azaozz (previous) (diff)

#23 @desrosj
3 years ago

After catching up here, my stance is that backporting to some degree is the correct move here. But I'm less clear on how far. Some rough thoughts in no particular order:

  • The main thing that made these changes required was supporting PHP 8 (though cross-version support for PHPUnit is a plus for any version).
  • PHP 8 (beta) support was added in WP 5.6.
  • I don't think there is any responsibility for us to consider situations where plugins support PHP < 5.6. This support change was made in Core 2 full years ago.
  • There seems to be a pretty big assumption here that plugins/extenders are testing against multiple branches of WordPress and PHP versions. While I'm sure some are, I think that many will be testing against trunk and maybe the latest 1 or 2 versions only.

While the changes proposed to the 5.1 and older branches are pretty minimal, I think I would prefer to start with only backporting through WP 5.2 as I'm not convinced it's necessary to go any further at this time.

At first I was going to suggest only backporting to WP 5.6 (when PHP 8 support was introduced), but WP 5.2 marked the beginning of the current PHP support policy with the exception of new versions of PHP released after the policy was created. That would allow extenders to test against 8 versions of WP (5.2-5.8 + trunk).

#24 @hellofromTonya
3 years ago

  • Keywords commit added

PR 1588 is now ready for backporting to WP 5.8 back to 5.2.

@desrosj @SergeyBiryukov is it possible to backport this week? Why? Would be good to publish the dev note early next (currently pending until the backports are done).

#25 @desrosj
3 years ago

  • Owner changed from johnbillion to desrosj

I'll be backporting this tomorrow with @hellofromTonya to introduce her to the backporting process.

jrfnl commented on PR #1588:


3 years ago
#26

FYI: I've made one tiny change still - changing the composer update to composer install in the test bootstrap messaging as the older WP versions still have a committed composer.lock file (which will not be changed in this backport).

#27 @desrosj
3 years ago

In 51838:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.8 branch.
See #53911.

#28 @desrosj
3 years ago

In 51839:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.7 branch.
See #53911.

#29 @desrosj
3 years ago

In 51840:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.6 branch.
See #53911.

#30 @hellofromTonya
3 years ago

In 51843:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.5 branch.
See #53911.

#31 @hellofromTonya
3 years ago

In 51844:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.4 branch.
See #53911.

#32 @hellofromTonya
3 years ago

In 51845:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.3 branch.
See #53911.

#33 @hellofromTonya
3 years ago

In 51846:

Build/Test Tools: Introduce the PHPUnit Polyfills package for easier cross branch testing.

This backports the PHPUnit Polyfills package and related test infrastructure changes to make it easier for developers to continue testing on multiple versions WordPress while adding tests for newer versions of PHP, which require more modern PHPUnit practices.

One of the changes included is the addition of wrappers for the new snake_case fixture methods in PHPUnit. This allows the native camelCase standard in PHPUnit to be used, but allows for developers to transition to the new naming conventions.

Props hellofromTonya, jrf, SergeyBiryukov, johnbillion, netweb, schlessera, jeherve, lucatume, desrosj.
Merges [51559,51560,51810-51813,51828] to the 5.2 branch.
See #53911.

#35 @hellofromTonya
3 years ago

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

Closing this ticket as there is not consensus to backport further than 5.2. If this changes, the ticket can be reopened.

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


3 years ago

jrfnl commented on PR #1589:


3 years ago
#37

Closing this as "won't fix".

pierlon commented on PR #1589:


3 years ago
#38

@jrfnl May I ask why this is closed? For the AMP for WordPress plugin we're currently working on using snake_case variants of some of the test fixtures provided by PHPUnit-Polyfills. We test with WP 4.9 up to trunk, but not having these changes in WP 5.1 and lower becomes problematic as we'd have to adapt our test suite to accommodate that.

jrfnl commented on PR #1589:


3 years ago
#39

@pierlon Please see the discussion in the Trac ticket. The short answer is that there just isn't enough support for backporting beyond WP 5.2.

If you like, I could see if I can get an okay to add a solution for this to WP Test Utils. No guarantee, but if you'd want that, please open feature request in that repo.

pierlon commented on PR #1589:


3 years ago
#40

Ah sorry, all caught up on the Trac ticket now 😄. I'll definitely open a feature request in that repo, thanks @jrfnl.

#41 follow-up: @bjorsch
3 years ago

Allow tests which need to be backported for the security test suite to be backported to older WP versions without the need to make significant changes to these tests to get them to run.

If I understand correctly, the intent of this is to allow for a test written for 5.9, that uses set_up(), to be backported to 5.8 and earlier without having to rename that to setUp().

If so, it looks like you got it exactly backwards in https://core.trac.wordpress.org/changeset/51838#file6. What you need for the tests to actually work is to have setUp() call set_up() rather than vice versa.

You can test this easily enough by adding a set_up() to some test in 5.8. You'll find it never gets called.

Probably the best solution would have been to just do like is in trunk after https://core.trac.wordpress.org/changeset/51561, let the polyfill library handle it (and make sure nothing forgets to call parent::setUp() and such, of course). But if you don't want to use the polyfill library directly for that for some reason (or in 5.1 and earlier where the polyfill library can't be loaded), you'd want to more or less copy how it's done in https://github.com/Yoast/PHPUnit-Polyfills/blob/develop/src/TestCases/TestCasePHPUnitLte7.php into a PHPUnit_Adapter_TestCase.

#42 @swissspidy
3 years ago

I just came here to report the same thing. I tried to make this work, but right now as a plugin developer it's not possible to run tests against these older branches because of that.

#43 @hellofromTonya
3 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for reporting @swissspidy and @bjorsch. Reopening and currently discussing with @jrf. Hope to have a solution later today.

#44 @jrf
3 years ago

@bjorsch @swissspidy Thanks for reporting. Sorry about that. At some point, you (in this case: me) can be too deep into things and stare yourself blind on something. We'll get a fix out soon.

Last edited 3 years ago by jrf (previous) (diff)

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


3 years ago
#45

  • Keywords has-patch has-unit-tests added; needs-patch removed

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

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

#46 in reply to: ↑ 41 @jrf
3 years ago

@bjorsch @swissspidy I've opened PR 1698 on GitHub with a proposed solution. If you have the chance, I would really appreciate you testing this PR.

I'll be opening PRs for the other branches as well once the build for this one has passed.

swissspidy commented on PR #1698:


3 years ago
#47

@jrfnl I think you also need to update includes/phpunit7/testcase.php

jrfnl commented on PR #1698:


3 years ago
#48

@swissspidy You're 100% correct. Thought of it multiple times while working on the patch and still forgot in the end. Fixing it up now.

jrfnl commented on PR #1698:


3 years ago
#49

(I should turn off Slack when working on code)

jrfnl commented on PR #1698:


3 years ago
#50

Done!

jrfnl commented on PR #1698:


3 years ago
#51

Thanks for testing @swissspidy !

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


3 years ago
#52

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

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


3 years ago
#53

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

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


3 years ago
#54

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

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


3 years ago
#55

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

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


3 years ago
#56

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

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


3 years ago
#57

Problem description:
The test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

I've explored the following potential options to solve this:

  1. Implement use of the PHPUnit Polyfills testcase.

This would require a change in the TestCase inheritance order and - to be on the safe side - changing the fixture method names throughout the WP Core test suite.
Such a change is larger than a helper backport warrants and is therefore not the preferred choice.

  1. Using the PHPUnit natively supported @beforeClass, @before, @after and @afterClass annotations to wire in the snake_case methods to be recognized as "hook callbacks" by PHPUnit.

I've investigated this option. Unfortunately, this would mean that the typical "parent" versus "child" method order for these methods would become reversed.
For example, the typical run order for a setUp() method is: first run parent::setUp(), next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run the parent::tearDown()).
The annotations, however, reverse that order as for the setUp() methods, the (snake_case) method with the annotation would get called first and then the PHPUnit setUp() would get called after (and visa versa for teardown).
As this could have unexpected side-effects, that option was discarded.
Note: using the annotations in combination with calling the camelCase parent from within the annotated snake_case method would mean the camelCase parent method would be called twice, once at the start, once at the end of the sequence, which, again, could have unintented side-effects, so that's another reason not to use this option.

  1. [CHOSEN SOLUTION] Adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.
As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.
Note: while it will be rare, there _may_ be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second.
For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method.
In those (rare) cases, the execution order of the code will now be changed, which may have side-effects.
A note to that effect will be added to the dev-note about the test changes.

Includes backporting wrappers for the assertPreConditions() and assertPostConditions() fixture methods. While rarely used, if we're doing an extra backport now anyway, we may as well make it feature complete for the fixture wrappers.

Trac ticket:

#58 @hellofromTonya
3 years ago

  • Owner changed from desrosj to hellofromTonya
  • Status changed from reopened to reviewing

Shifting ownership to me for backporting the test forward-compatibility layer fixes.

#59 @hellofromTonya
3 years ago

In 51861:

Build/Test Tools: Fix test forward-compatibility layer.

In [51838], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51838].

Props bjorsch, swisspidy, jrf, hellofromTonya.
See #53911.

#61 @hellofromTonya
3 years ago

In 51862:

Build/Test Tools: Fix test forward-compatibility layer.

In [51839], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51839].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

anomiex commented on PR #1698:


3 years ago
#63

Looks like it works for us too, although we may be running into an execution order issue in a handful of tests.

#65 @hellofromTonya
3 years ago

In 51864:

Build/Test Tools: Fix test forward-compatibility layer.

In [51843], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51843].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

#66 @hellofromTonya
3 years ago

In 51863:

Build/Test Tools: Fix test forward-compatibility layer.

In [51840], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51840].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

#68 @hellofromTonya
3 years ago

In 51865:

Build/Test Tools: Fix test forward-compatibility layer.

In [51844], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51844].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

#70 @hellofromTonya
3 years ago

In 51866:

Build/Test Tools: Fix test forward-compatibility layer.

In [51845], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51845], [51848].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

#72 @hellofromTonya
3 years ago

In 51867:

Build/Test Tools: Fix test forward-compatibility layer.

In [51846], the test wrapper methods were not being called due to the names not being recognized as supported PHPUnit "hook" names for fixtures.

This commit:

  • Fixes the problem by adding extra camelCase wrappers to the WP_UnitTestCase to call the methods in the right order.
  • Adds wrappers for the assertPreConditions() and assertPostConditions() fixture methods to make the backport feature complete for the fixture wrappers.

Test wrapper methods call fix:

By adding method overloads for the PHPUnit native camelCase fixture methods and letting those call the (camelCase) parent method first and only calling the snake_case fixture methods after, the snake_case methods can be supported and the typical run order safeguarded.

As not all test classes will have declared snake_case fixture methods, the snake_case fixture methods are also declared in the WP_UnitTestCase. Why? This prevents having to wrap these method calls in method_exists() conditions checking for the existence of the snake_case methods in an unknown Test child class. And with the normal inheritance rules in combination with calling the method using static, the right method will be called anyway without fatal "calling undeclared method" errors.

Note: While it will be rare, there may be cases where a test class does not adhere to the normal execution order for fixtures, i.e. for the setup methods, parent first, own code second; and for the teardown methods, own code first, parent second. For example a test class which has "some code - parent::setUp() call - some more code" in their setUp() method. In those (rare) cases, the execution order of the code will now be changed, which may have side-effects. This rare case will be identified in the dev note.

Follow-up to [51846], [51847].

Props bjorsch, swissspidy, jrf, hellofromTonya.
See #53911.

jrfnl commented on PR #1698:


3 years ago
#74

Looks like it works for us too, although we may be running into an execution order issue in a handful of tests.

@anomiex Thanks for letting us know. Just to confirm: the execution order issues you are seeing are related to the fixture methods being run in an atypical order in your tests ? If so, yes, that is as noted above and will be included in the dev-note.

If there is an execution order issue somewhere else, I'd be very interested to hear more.

#75 @hellofromTonya
3 years ago

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

Reclosing as [51861-51867] backports resolve the issue of the test wrapper methods not being called.

anomiex commented on PR #1698:


3 years ago
#76

Just to confirm: the execution order issues you are seeing are related to the fixture methods being run in an atypical order in your tests ?

That is what I was referring to, yes.

It turned out that the test class's tear down method was neglecting to call the parent at all, and some of the tests had logic errors that were masked by state left over from earlier tests that would normally be reset by WP_UnitTestCase_Base. This PR resulted in WP_UnitTestCase_Base's tear down running (because we renamed tearDown() to tear_down() in our tests), exposing those logic errors that had been hidden. Which is to the good, I rewrote those tests of ours to do what they were intended to do.

jrfnl commented on PR #1698:


3 years ago
#77

@anomiex Thanks for confirming and yes, those all sound like bugs in those tests and not bugs in this PR.

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


2 years ago

Note: See TracTickets for help on using tickets.