WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 3 weeks ago

#54183 assigned task (blessed)

Tests: decide on how to handle deprecations in PHPUnit

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-dev-note 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 4 weeks 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 4 weeks ago.
Screenshot of how deprecations are handled in PHPUnit 9.5.10
54183-option-2.patch (2.6 KB) - added by jrf 4 weeks ago.
Patch for option 2
54183-option-3.patch (10.5 KB) - added by costdev 4 weeks ago.
Skip risky tests.

Download all attachments as: .zip

Change History (27)

@jrf
4 weeks ago

Screenshot of how deprecations are handled in PHPUnit 9.5.9

@jrf
4 weeks ago

Screenshot of how deprecations are handled in PHPUnit 9.5.10

#1 @jrf
4 weeks ago

  • Description modified (diff)

#2 @jrf
4 weeks ago

  • Description modified (diff)

#3 @jrf
4 weeks ago

  • Keywords needs-dev-note php81 added

#4 @jrf
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

Patch for option 2

#7 @jrf
4 weeks ago

  • Milestone changed from Awaiting Review to 5.9

#8 in reply to: ↑ 5 @SergeyBiryukov
4 weeks 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.


4 weeks ago

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


4 weeks ago

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


4 weeks ago

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
4 weeks 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
4 weeks 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 4 weeks ago by jrf (previous) (diff)

#14 @netweb
4 weeks 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
4 weeks 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 💯

#16 @prbot
4 weeks ago

ntwb commented on PR #1709:

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
4 weeks 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
4 weeks 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
4 weeks ago

Skip risky tests.

#19 @costdev
4 weeks 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.
Last edited 4 weeks ago by costdev (previous) (diff)

#20 @jrf
4 weeks 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 weeks ago

#23 @hellofromTonya
3 weeks ago

  • Keywords commit removed

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

Note: See TracTickets for help on using tickets.