Tests: decide on how to handle deprecations in PHPUnit

[jrf]

PHPUnit just released version 9.5.10 and 8.5.21.

This contains a particular (IMO breaking) change which I believe we should have a think about how to handle:

Changed

• PHPUnit no longer converts PHP deprecations to exceptions by default (configure convertDeprecationsToExceptions="true" to enable this)
• The PHPUnit XML configuration file generator now configures convertDeprecationsToExceptions="true"

Let's unpack this:

Previously (PHPUnit < 9.5.10/8.5.21), if PHPUnit would encounter a PHP native deprecation notice, it would:

1. Show a test which causes a deprecation notice to be thrown as "errored",
2. Show the first deprecation notice it encountered and
3. PHPUnit would exit with a non-0 exit code (2), which will fail a CI build.

As of PHPUnit 9.5.10/8.5.21, if PHPUnit encounters a PHP native deprecation notice, it will no longer do so. Instead PHPUnit will:

1. Show a test which causes a PHP deprecation notice to be thrown as "risky",
2. Show the all deprecation notices it encountered and
3. PHPUnit will exit with a 0 exit code, which will show a CI build as passing.

Options

IMO, there are three options here:

1. Leave as is. Deprecation notices means something will still work for the time being and will not change the behaviour of WordPress.

Pro:

• All deprecations caused by a particular test will be shown, not just the first.

Con:

• As CI builds pass, deprecations may go unnoticed, while they will eventually still need to be fixed as in the next PHP major will become errors.
• More deprecations will stay in WP, causing more churn from end-users reporting notices seen.

2. Revert to the previous behaviour by adding convertDeprecationsToExceptions="true" to the PHPUnit configuration.

Pro:

• Deprecation notices can not go unnoticed as CI builds will fail on them.

Con:

• You will only see the first deprecation notice for a test, there may be more issues hiding behind a deprecation.
• (minor) The PHPUnit configuration will no longer validate against the XSD scheme on PHPUnit 5/6/7.

3. Add failOnRisky="true" to the PHPUnit configuration.

Pro:

• All deprecations caused by a particular test will be shown, not just the first.
• Deprecation notices will cause the CI builds to fail, so can not go unnoticed.

Con:

• The WP test suite contains a number of other tests which are currently marked as "risky". These will now also cause a build to fail.

I personally favour option 2 or 3, but would love to see some more opinions on this.

Pinging @hellofromtonya @johnbillion @sergey @netweb for visibility.

Changelogs:

• ​https://github.com/sebastianbergmann/phpunit/blob/9.5/ChangeLog-8.5.md
• ​https://github.com/sebastianbergmann/phpunit/blob/9.5/ChangeLog-9.5.md

Related: #53363

[jrf]

Screenshot of how deprecations are handled in PHPUnit 9.5.9

[jrf]

Screenshot of how deprecations are handled in PHPUnit 9.5.10

[jrf]

I'm tagging this with needs-dev-note as I believe that - independently of what course of action is taken for WP itself -, this PHPUnit change needs a mention in the PHP 8.1 dev-note so people are aware that they may need to update their plugin/theme PHPUnit configuration.

[costdev]

Option 1
If there's a lot of deprecation notices, but the CI still passes, the notices will likely be unnoticed/ignored. As the next major would result in errors, we'd ultimately just be delaying the inevitable and increasing our workload when the time comes.

Option 2
Reverting to previous behaviour keeps things consistent with contributor experience. It's worth doing, but it's a shame that only the first notice shows.

Option 3
This is the best option IMHO, but requires some work to resolve risky tests in Core.

We could do a review of risky tests to get an idea of the work involved in implementing Option 3. Until it's implemented, I'd suggest that we implement Option 2 as soon as possible to prevent any deprecations making their way into Core.

[jrf]

I've uploaded an initial patch to implement option 2.

Option three needs further investigation, so can be a future iteration on the initial patch.

[jrf]

Patch for option 2

[SergeyBiryukov]

Replying to costdev:

Option 3
This is the best option IMHO, but requires some work to resolve risky tests in Core.

We could do a review of risky tests to get an idea of the work involved in implementing Option 3. Until it's implemented, I'd suggest that we implement Option 2 as soon as possible to prevent any deprecations making their way into Core.

I tend to agree. I've been meaning to review those risky tests for a while :)

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

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

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

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

PHPUnit just released version 9.5.10 and 8.5.21.

This contains a particular (IMO breaking) change:

• PHPUnit no longer converts PHP deprecations to exceptions by default (configure convertDeprecationsToExceptions="true" to enable this)

Let's unpack this:

Previously (PHPUnit < 9.5.10/8.5.21), if PHPUnit would encounter a PHP native deprecation notice, it would:

1. Show a test which causes a deprecation notice to be thrown as "errored",
2. Show the first deprecation notice it encountered and
3. PHPUnit would exit with a non-0 exit code (2), which will fail a CI build.

As of PHPUnit 9.5.10/8.5.21, if PHPUnit encounters a PHP native deprecation notice, it will no longer do so. Instead PHPUnit will:

1. Show a test which causes a PHP deprecation notice to be thrown as "risky",
2. Show the all deprecation notices it encountered and
3. PHPUnit will exit with a 0 exit code, which will show a CI build as passing.

This commit reverts PHPUnit to the previous behaviour by adding convertDeprecationsToExceptions="true" to the PHPUnit configuration.
It also adds the other related directives for consistency.

Refs:

• sebastianbergmann/phpunit@9.5/ChangeLog-8.5.md
• sebastianbergmann/phpunit@9.5/ChangeLog-9.5.md

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

Via https://core.trac.wordpress.org/attachment/ticket/54183/54183-option-2.patch

cc @SergeyBiryukov @jrfnl

[netweb]

I'm seeing an instance of this in the PR I created a few hours before this ticket was created:

​https://github.com/WordPress/wordpress-develop/pull/1709

PHPUnit Tests currently fail on PHP 7.2, 7.3, 7.4, & 8.0 which are CI runs that use PHPUnit 9.5.10 and 8.5.21:

There was 1 failure:

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

The test file source is ​here, migrated from ​here via r51852

I've created a PR with the option-2 patch ​https://github.com/WordPress/wordpress-develop/pull/1710

Maybe go for option-2 now whilst further researching option-3?

[jrf]

Replying to netweb:

I'm seeing an instance of this in the PR I created a few hours before this ticket was created:

​https://github.com/WordPress/wordpress-develop/pull/1709

PHPUnit Tests currently fail on PHP 7.2, 7.3, 7.4, & 8.0 which are CI runs that use PHPUnit 9.5.10 and 8.5.21:

Ouch.. yes, that's what I meant by this being a breaking change. Shouldn't really have been released in a patch version of PHPUnit IMO.

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

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

phpunit --migrate-configuration

I've created a PR with the option-2 patch ​https://github.com/WordPress/wordpress-develop/pull/1710

Maybe go for option-2 now whilst further researching option-3?

Yes, looking at the responses so far, I think that's the way to go: commit the option 2 patch ASAP and then look at option 3 potentially for the future.

[netweb]

54183-option-2.patch​ looks good via ​https://github.com/WordPress/wordpress-develop/pull/1710

Aside: @SergeyBiryukov mentioned that there is also an incoming commit to fix the test, so potentially with this only single test failure, the urgency of committing this fix may be lessened, will leave this for @SergeyBiryukov to confirm and decide.

[netweb]

Replying to jrf:

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

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

phpunit --migrate-configuration

Noted, thanks, will close the PR and wait for PHPUnit v10.x 💯

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

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

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

phpunit --migrate-configuration

[SergeyBiryukov]

In 51871:

Build/Test Tools: Update PHPUnit configuration for PHPUnit 9.5.10/8.5.21+.

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

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

Let's unpack this:

Previously (PHPUnit < 9.5.10/8.5.21), if PHPUnit would encounter a PHP native deprecation notice, it would:

1. Show a test which causes a deprecation notice to be thrown as "errored",
2. Show the first deprecation notice it encountered and
3. PHPUnit would exit with a non-0 exit code (2), which will fail a CI build.

As of PHPUnit 9.5.10/8.5.21, if PHPUnit encounters a PHP native deprecation notice, it will no longer do so. Instead PHPUnit will:

1. Show a test which causes a PHP deprecation notice to be thrown as "risky",
2. Show the all deprecation notices it encountered and
3. PHPUnit will exit with a 0 exit code, which will show a CI build as passing.

This commit reverts PHPUnit to the previous behaviour by adding convertDeprecationsToExceptions="true" to the PHPUnit configuration. It also adds the other related directives for consistency.

Props jrf, netweb, costdev, SergeyBiryukov.
See #54183.

[SergeyBiryukov]

In 51872:

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

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

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

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

There was 1 failure:

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

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

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

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

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

[costdev]

Skip risky tests.

[costdev]

Attaching 54183-option-3.patch for review.

• Skips risky tests.
• Influenced by ​this implementation.
• Adds if ( ! empty( $tags ) ) {} check to WP_Test_REST_Tags_Controller()->test_get_terms_post_args_paging() else skips the test.

[jrf]

@costdev Hah! I hadn't looked at the other risky tests when I opened this ticket, but seeing your patch, I think we need to move the discussion about the risky tests to #53363, where there is already another patch open for this. Also see the original ticket which introduced this.

In particular see comment #53363#24 and the few comments in response to it about these particular tests.

Merged via https://core.trac.wordpress.org/changeset/51871
Follow up https://core.trac.wordpress.org/changeset/51872

[hellofromTonya]

Removing commit as ready for commit patches have already been committed.

[hellofromTonya]

With 5.9 RC1 tomorrow, moving remaining work to 6.0.

@jrf is a dev note needed for 5.9?

[jrf]

@jrf is a dev note needed for 5.9?

Yes as per my previous comment, this should be included in the PHP 8.1 dev-note.

I'm tagging this with needs-dev-note as I believe that - independently of what course of action is taken for WP itself -, this PHPUnit change needs a mention in the PHP 8.1 dev-note so people are aware that they may need to update their plugin/theme PHPUnit configuration.

[hellofromTonya]

Thanks @jrf. Curious.. is the PHP 8.1 dev-note on your TODO?

[jrf]

Replying to hellofromTonya:

Thanks @jrf. Curious.. is the PHP 8.1 dev-note on your TODO?

What with the WP 5.9 release having been delayed to coincide with my break: no.

Happy to give someone else some pointers and look things over, but I won't be spending days writing it during my holidays.

[costdev]

This ticket was discussed during the recent bug scrub.

As we haven't made any further progress on this ticket and focuses are currently elsewhere for this release cycle, I'm pre-emptively moving this to 6.1 to take it off the milestone ahead of RC1.

[chaion07]

Thanks @jrf for reporting this. We reviewed this ticket during a ​recent bug-scrub session. Based on the feedback received, we are updating the ticket with the following changes:

1. Removing the keyword needs-dev-note since ​this Dev Note covers the ticket in discussion It can always be added again later if needed.
2. Punting the ticket to Future Release.

Cheers!

Props to @mikeschroder @jrf @mukesh27 @robinwpdeveloper for the discussion

[SergeyBiryukov]

Replying to SergeyBiryukov:

Replying to costdev:

Option 3
This is the best option IMHO, but requires some work to resolve risky tests in Core.

We could do a review of risky tests to get an idea of the work involved in implementing Option 3. Until it's implemented, I'd suggest that we implement Option 2 as soon as possible to prevent any deprecations making their way into Core.

I tend to agree. I've been meaning to review those risky tests for a while :)

Just noting that empty REST API tests previously marked as risky were addressed in [53921] and [54058] / #41463.