#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 | Owned by: | 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 )
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
@
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)
#3
@
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 theWP_UnitTestCase
class. These fall through to the camelCase methods in theWP_UnitTestCase_Base
class. - Add
set_up()
,tear_down()
etc wrappers to the other WP native TestCases within thetests/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
@
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
@
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.
#10
follow-up:
↓ 11
@
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
@
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:
↓ 14
↓ 16
@
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.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
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
@
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 :)
#16
in reply to:
↑ 13
;
follow-up:
↓ 18
@
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:
- 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 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:
- The tests for WP 5.9 are running on PHPUnit 9.5.
- 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
@
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:
↓ 19
↓ 20
@
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
@
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
@
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
@
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
@
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.
#23
@
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
@
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
@
3 years ago
- Owner changed from johnbillion to desrosj
I'll be backporting this tomorrow with @hellofromTonya to introduce her to the backporting process.
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).
hellofromtonya commented on PR #1588:
3 years ago
#34
Committed via changesets [51838-51840,51843-51848] https://core.trac.wordpress.org/log/?revs=51838-51840,51843-51848
#35
@
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
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.
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.
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:
↓ 46
@
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
@
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
@
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
@
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.
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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
@
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
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.
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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:
- 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.
- 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 asetUp()
method is: first runparent::setUp()
, next run your own code (and the reverse for the tear downs, i.e. first run your own code, then run theparent::tearDown()
).
The annotations, however, reverse that order as for thesetUp()
methods, the (snake_case) method with the annotation would get called first and then the PHPUnitsetUp()
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.
- [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 theWP_UnitTestCase
. This prevents having to wrap these method calls inmethod_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 usingstatic
, 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 theirsetUp()
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
@
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.
hellofromtonya commented on PR #1698:
3 years ago
#60
Committed in changeset https://core.trac.wordpress.org/changeset/51861.
hellofromtonya commented on PR #1700:
3 years ago
#62
Committed via changeset https://core.trac.wordpress.org/changeset/51862.
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.
hellofromtonya commented on PR #1701:
3 years ago
#64
Committed via changeset https://core.trac.wordpress.org/changeset/51863.
hellofromtonya commented on PR #1702:
3 years ago
#67
Committed via changeset https://core.trac.wordpress.org/changeset/51864.
hellofromtonya commented on PR #1703:
3 years ago
#69
Committed via changeset https://core.trac.wordpress.org/changeset/51865.
hellofromtonya commented on PR #1704:
3 years ago
#71
Committed via changeset https://core.trac.wordpress.org/changeset/51866.
hellofromtonya commented on PR #1705:
3 years ago
#73
Committed via changeset https://core.trac.wordpress.org/changeset/51867.
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
@
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.
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.
3 years ago
#77
@anomiex Thanks for confirming and yes, those all sound like bugs in those tests and not bugs in this PR.
The following is a matrix of the popular plugins.
Plugins with >= PHP 5.4 minimum:
Plugins with < PHP 5.4 minimum:
Plugins with unknown PHP minimum: