[wp-trac] [WordPress Trac] #50639: [PHP 8] Add @requires annotations for PHPUnit tests that assert engine-enforced constraints

[WordPress Trac]

#50639: [PHP 8] Add @requires annotations for PHPUnit tests that assert engine-
enforced constraints
------------------------------------------+---------------------
 Reporter:  ayeshrajans                   |       Owner:  (none)
     Type:  defect (bug)                  |      Status:  new
 Priority:  normal                        |   Milestone:  5.6
Component:  Build/Test Tools              |     Version:
 Severity:  normal                        |  Resolution:
 Keywords:  php8 has-patch needs-refresh  |     Focuses:
------------------------------------------+---------------------
Changes (by jrf):

 * keywords:  php8 has-patch => php8 has-patch needs-refresh


Comment:

 Hi @ayeshrajans Thanks for opening this ticket. I've had a look at this
 ticket and got some feedback for you to consider.

 > @required annotations are not supported in earlier versions such as 5.x
 and 6.x, which WordPress still uses for older PHP versions. For PHP 8,
 PHPUnit 7.5 will be used, so using @required annotations is a progressive
 update.

 I don't know where you got this information from, but it's incorrect.

 The `@requires` annotations have been supported for the longest time,
 including in PHPUnit 5.x and 6.x and even in
 [https://phpunit.de/manual/4.8/en/incomplete-and-skipped-tests.html
 #incomplete-and-skipped-tests.skipping-tests-using-requires PHPUnit 4.x].

 The only thing which I can think of regarding `@requires` annotations
 which may not have been supported before PHPUnit 7.x, is the use of the
 `<` operator.

 This would need some quick testing with older PHPUnit versions to make
 sure they completely ignore a `@requires` annotation when they can't
 interpret it (in contrast to skipping the test when they can't interpret
 the `@requires`).
 I trust that will be fine, but would like to see it confirmed.

 > In current PHP 8 builds, there are few test failures on tests that are
 supposed to test how functions behave when they encounter unexpected
 values. In PHP 8, it is not necessary to have these tests because they are
 validated internally by the engine now.

 I don't think this is correct.
 The tests are verifying that a "doing it wrong" notice is being thrown
 when the function is called incorrectly, but that the function will still
 handle the provided input as correctly as possible.

 Disabling those tests on PHP 8 hides a problem, i.e. the function will no
 longer throw a notice and handle things correctly, it will now cause a
 white screen of death due to a fatal error.

 This is a backward-compatibility break and WP will either need to put code
 in place to maintain the original behaviour of the function or document
 that the behaviour in certain circumstances is different on PHP 8.


 > Adds `@requires extension gd` to mark that the test requires gd
 extension. This is to prevent "jpeg support required" test failures on
 current PHP nightly builds. I think failing a test when the test
 environment does not support the extension is not appropriate and we
 should ideally skip that test instead. Fixing test-failures to mark tests
 skipped is out of this ticket.

 I 100% agree with the reasoning about skipping rather than failing tests
 and am seriously wondering why this was never changed before now.

 Regarding the implementation though, I would suggest an alternative route:

 Consider this test in `tests/image/intermediateSize.php`:
 {{{#!php
 <?php
         function test_make_intermediate_size_width() {
                 if ( ! function_exists( 'imagejpeg' ) ) {
                         $this->fail( 'jpeg support unavailable' );
                 }

                 $image = image_make_intermediate_size( DIR_TESTDATA .
 '/images/a2-small.jpg', 100, 0, false );

                 $this->assertInternalType( 'array', $image );
         }
 }}}

 As `@requires` annotations are supported in all used PHPUnit versions and
 the `@requires` annotation supports functions, I'd suggest changing this
 to:
 {{{#!php
 <?php
         /**
          * @requires function imagejpeg
          */
         function test_make_intermediate_size_width() {
                 $image = image_make_intermediate_size( DIR_TESTDATA .
 '/images/a2-small.jpg', 100, 0, false );

                 $this->assertInternalType( 'array', $image );
         }
 }}}

 Reasoning:
 1. Test skipping will be consistent across PHP/PHPUnit versions.
 2. Optimal use of PHPUnit native functionality.
 3. Only skipped when the most specific requirement is met, i.e., the
 function doesn't exist rather than the extension isn't loaded.

 I also believe this could/should be applied in more places as a quick
 search of the test code base shows quite a number of places where either
 `$this->fail()` or `$this->markTestSkipped()` is used in combination with
 an extension or function check.

 Hope this helps ;-)

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