Make WordPress Core

Opened 3 years ago

Last modified 21 months ago

#54183 assigned task (blessed)

Tests: decide on how to handle deprecations in PHPUnit

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: php81 has-patch has-unit-tests
Focuses: Cc:

Description (last modified by 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.

Screenshot of how deprecations are handled in PHPUnit 9.5.9

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.

Screenshot of how deprecations are handled in PHPUnit 9.5.10

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:

Related: #53363

Attachments (4)

deprecations-in-phpunit-9.5.9.png (31.2 KB) - added by jrf 3 years ago.
Screenshot of how deprecations are handled in PHPUnit 9.5.9
deprecations-in-phpunit-9.5.10.png (71.9 KB) - added by jrf 3 years ago.
Screenshot of how deprecations are handled in PHPUnit 9.5.10
54183-option-2.patch (2.6 KB) - added by jrf 3 years ago.
Patch for option 2
54183-option-3.patch (10.5 KB) - added by costdev 3 years ago.
Skip risky tests.

Download all attachments as: .zip

Change History (36)

@jrf
3 years ago

Screenshot of how deprecations are handled in PHPUnit 9.5.9

@jrf
3 years ago

Screenshot of how deprecations are handled in PHPUnit 9.5.10

#1 @jrf
3 years ago

  • Description modified (diff)

#2 @jrf
3 years ago

  • Description modified (diff)

#3 @jrf
3 years ago

  • Keywords needs-dev-note php81 added

#4 @jrf
3 years ago

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.

#5 follow-up: @costdev
3 years ago

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.

#6 @jrf
3 years ago

  • Keywords has-patch added; needs-patch removed

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
3 years ago

Patch for option 2

#7 @jrf
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#8 in reply to: ↑ 5 ; follow-up: @SergeyBiryukov
3 years ago

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 :)

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


3 years ago
#9

  • Keywords has-unit-tests added

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.

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


3 years ago

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


3 years ago
#11

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

#12 follow-up: @netweb
3 years ago

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?

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

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.

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

#14 @netweb
3 years ago

  • Keywords commit added; 2nd-opinion removed
  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

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.

#15 in reply to: ↑ 13 @netweb
3 years ago

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 💯

ntwb commented on PR #1709:


3 years ago
#16

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

#17 @SergeyBiryukov
3 years ago

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.

#18 @SergeyBiryukov
3 years ago

In 51872:

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

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

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

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

There was 1 failure:

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

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

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

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

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

@costdev
3 years ago

Skip risky tests.

#19 @costdev
3 years ago

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.
Version 0, edited 3 years ago by costdev (next)

#20 @jrf
3 years ago

@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.

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


3 years ago

#23 @hellofromTonya
3 years ago

  • Keywords commit removed

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

#24 follow-up: @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to 6.0

With 5.9 RC1 tomorrow, moving remaining work to 6.0.

@jrf is a dev note needed for 5.9?

#25 in reply to: ↑ 24 @jrf
2 years ago

@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.

#26 follow-up: @hellofromTonya
2 years ago

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

#27 in reply to: ↑ 26 @jrf
2 years ago

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.

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


2 years ago

#29 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

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.

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


21 months ago

#31 @chaion07
21 months ago

  • Keywords needs-dev-note removed
  • Milestone changed from 6.1 to Future Release

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

#32 in reply to: ↑ 8 @SergeyBiryukov
21 months ago

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.

Note: See TracTickets for help on using tickets.