[wp-trac] [WordPress Trac] #54183: Tests: decide on how to handle deprecations in PHPUnit

[WordPress Trac]

#54183: Tests: decide on how to handle deprecations in PHPUnit
-------------------------------------+------------------------------
 Reporter:  jrf                      |       Owner:  (none)
     Type:  task (blessed)           |      Status:  new
 Priority:  normal                   |   Milestone:  Awaiting Review
Component:  Build/Test Tools         |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  needs-patch 2nd-opinion  |     Focuses:
-------------------------------------+------------------------------
Description changed by jrf:

Old description:

> 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.
>
> [[Image(ticket:deprecations-in-phpunit-9.5.9.png)]]
> https://snipboard.io/OsTNz8.jpg
>
> 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.
>

> [[Image(ticket:deprecations-in-phpunit-9.5.10.png)]]
> https://snipboard.io/Bsdmau.jpg
>
> == 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

New description:

 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.

 [[Image(ticket:54183:deprecations-in-phpunit-9.5.9.png)]]
 https://snipboard.io/OsTNz8.jpg

 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.


 [[Image(ticket:54183:deprecations-in-phpunit-9.5.10.png)]]
 https://snipboard.io/Bsdmau.jpg

 == 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

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54183#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform