PHPUnit 7.x support

[ayeshrajans]

PHPUnit 7.0.0 was released today, and it is not compatible with out PHPUnit-compat. I'm not trying to be a push-over, but I think it's good if we can keep up with with the latest testing frameworks.

Apart from the changes listed in ​https://phpunit.de/announcements/phpunit-7.html , we now need to make a few changes to our WP tests and ideally drop support for very old versions.

• phpuit6-compat: class PHPUnit_Util_Test extends PHPUnit\Util\Test. In PHPUnit 7, PHPUnit\Util\Test is declared final. It is not necessary to extend this class, so one could work this around by simply not extending this class (e.g ​https://github.com/Ayesh/wordpress-develop/commit/23a4b2daa823ecc6b9979815b735c9c95f223a01)
• WP_PHPUnit_Util_Getopt class extends the now-final PHPUnit_Util_Getopt

There are several other places that we have to make improvements.

I hope we can make this a call for us to clean up our PHPUnit implementation and perhaps make this easier to manage.

I would like to get your opinion on making the PHPUnit integration a separate composer package (we already install PHPUnit as a composer package) and require the corresponding version to run the test suite.

This package will roughly contain the contents of ./tests/phpunit/includes directory, and will provide fixtures and extend PHPUnit framework classes as necessary. We can have multiple versions where we support PHPUnit 3.x/4.x backports, 5.x compat files, and 6.x/7.x fixes as necessary, with Composer resolving package versions properly.

[jorbin]

PHPunit 7 only support 7.1+ which is what ​less than 7% of all WordPress installs use. We can't only run our tests on versions of PHP that so few users use, so whatever we need to do to support it needs to be done in a way that is backwards compatible to PHPUnit 3.6 (the last version that supported 5.2).

A glance at the change in ​https://github.com/Ayesh/wordpress-develop/commit/23a4b2daa823ecc6b9979815b735c9c95f223a01 seems like it would be fine to do. Do you know what we would need to do for WP_PHPUnit_Util_Getopt ?

[ayeshrajans]

PHPUnit 5.x supported ends today, leaving users with 6.x and 7.x versions, all of which require PHP 7.0+. From the same install statistics, PHP 5.2 users made up only 3.5% of all installs.

If we start to require PHP 5.3 as the minimum version (which is already EOL and receives no support from php.net or any OS vendor that I know off), we can use phpunit ^4.8 and remove back-ports to 3.x versions which would otherwise trigger test errors.

PHPUnit 8.x has the roadmap that their interfaces will fully utilize PHP 7.1 parameter type hints and return types, making it highly unlikely that we will ever be able to use PHPUnit 8 with the current test fixtures/extensions unless we split them into sub packages and add a compat layer.

[ayeshrajans]

As for WP_PHPUnit_Util_Getopt, I don't have a workaround yet. This class is only used during test bootstrap (and not as test listeners, assertions, fixtures, etc), so we have room to refactor this without affecting any existing test.

[soulseekah]

PHPUnit_Util_Test has indeed always been used for the getTickets method, which in turn uses parseTestMethodAnnotations from PHPUnit\Util\Test. It is, thus, safe to remove the inheritance.

However, PHPUnit 7 introduces return type hinting, which is only available in PHP 7.0+. One such method, for example is WP_UnitTestCase::assertFalse which now hints at returning a void. The only way I know of to handle this in both 7.0+ and older versions is to copy the class into a PHP7+ version and load it dynamically. This is not going to fly, especially seeing that PHPUnit6 is more than okay for the time being.

[dingo_bastard]

Just to clarify, for unit tests to pass, we need to use PHPUnit 6? I tried running tests and I got

PHP Fatal error: Class PHPUnit_Util_Test may not inherit from final class (PHPUnit\Util\Test) in /vagrant-local/www/wordpress-develop/tests/phpunit/includes/phpunit6-compat.php on line 18

error. Will downgrading the PHPUnit to v6 get rid of this?

[soulseekah]

Replying to dingo_bastard:

Just to clarify, for unit tests to pass, we need to use PHPUnit 6? I tried running tests and I got

PHP Fatal error: Class PHPUnit_Util_Test may not inherit from final class (PHPUnit\Util\Test) in /vagrant-local/www/wordpress-develop/tests/phpunit/includes/phpunit6-compat.php on line 18

error. Will downgrading the PHPUnit to v6 get rid of this?

Yes, that's the point. PHPUnit 7 and up will probably not be supported for years to come. The PHP 7.x return hinting is just too much for our 5.x files.

composer require --dev phpunit/phpunit:^6

[dingo_bastard]

Yeah, I just noticed that in my project I have phpunit 6.4 but I also had globally installed phpunit 7. Thanks!

[jipmoors]

Implementing PHPUnit 7 compatibility

[jipmoors]

In the patch I have reduce complexity and dependencies wherever possible.

Elements that had to be touched:
The new file "abstract-testcase.php " contains the contents of "testcase.php" without the method that is being overridden en gave an argument declaration error.

Removed the dependency in the PHPUnit_Util_Test class, as it only needs to use the PHPUnit\Util\Test::parseTestMethodAnnotations which is a public static method, thus callable from everywhere.

Added a speed-trap loader, which determines what speed-trap class needs to be loaded for the current PHPUnit version.

Fixed a test.
Fixed the type of variable that is being used to show what bug is being reported, this was defaulted to 'null' instead of an empty string.

[SergeyBiryukov]

In 42799:

Tests: Correct assertion in Tests_WP_Customize_Setting::test_constructor_with_args().

Props jipmoors.
See #30988, #43218.

[netweb]

Patch 43218.2.diff​ is a refresh of Provide_PHPUnit_7_compatibility.patch​ with the patch hunk removed that was committed in [42799]

[netweb]

Testing 43218.2.diff​ in Travis CI

• ​https://github.com/ntwb/wordpress/pull/14
• ​https://github.com/ntwb/wordpress/pull/15 (Includes Travis CI update to use PHPUnit 7.x)

[netweb]

Replying to netweb:

Testing 43218.2.diff​ in Travis CI

• ​https://github.com/ntwb/wordpress/pull/14

Fails the PHP 5.2 Job: ​https://travis-ci.org/ntwb/wordpress/jobs/351128113#L436 (all other jobs pass)

• Warning: Unexpected character in input: '\' (ASCII=92) state=1 in /home/travis/build/ntwb/wordpress/tests/phpunit/includes/bootstrap.php on line 111
• Parse error: syntax error, unexpected T_STRING in /home/travis/build/ntwb/wordpress/tests/phpunit/includes/bootstrap.php on line 111

• ​https://github.com/ntwb/wordpress/pull/15 (Includes Travis CI update to use PHPUnit 7.x)

The PHP ​7.1, ​7.2, and nightly (log unavailable, same reason expected though) with:

• PHP Fatal error: Declaration of SpeedTrapListener::addError(PHPUnit_Framework_Test $test, Exception $e, $time) must be compatible with PHPUnit\Framework\TestListener::addError(PHPUnit\Framework\Test $test, Throwable $t, float $time): void in /home/travis/build/ntwb/wordpress/tests/phpunit/includes/speed-trap-listener.php on line 7
• Fatal error: Declaration of SpeedTrapListener::addError(PHPUnit_Framework_Test $test, Exception $e, $time) must be compatible with PHPUnit\Framework\TestListener::addError(PHPUnit\Framework\Test $test, Throwable $t, float $time): void in /home/travis/build/ntwb/wordpress/tests/phpunit/includes/speed-trap-listener.php on line 7

There are a bunch of database errors displayed when PHPUnit is invoked for the PHP 7.1 & 7.2 jobs:

• ​https://travis-ci.org/ntwb/wordpress/jobs/351130136#L858
• WordPress database error Table 'wordpress_tests.wptests_options' doesn't exist for query SELECT autoload FROM wptests_options WHERE option_name = '_site_transient_update_themes' made by require_once('wp-settings.php'), register_theme_directory, wp_clean_themes_cache, delete_site_transient, delete_site_option, delete_network_option, delete_option
• These do not 'fail' the job and will need further investigation also

[SergeyBiryukov]

43218.3.diff​ should fix all the failures. Tested on PHP 5.2.17 with PHPUnit 3.6.12 and PHP 7.2.2 with PHPUnit 7.0.2.

Was going to commit, but it was blocked by a pre-commit hook:

Commit failed (details follow):
Commit blocked by pre-commit hook (exit code 1) with output:

***********************************
PHP error in: trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php:

Parse error: syntax error, unexpected ':', expecting ';' or '{' in
 trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php on line 57
Errors parsing trunk/tests/phpunit/includes/phpunit7/speed-trap-listener.php

The line in question is:

public function addError( PHPUnit\Framework\Test $test, Throwable $t, float $time ): void {

Apparently the linter on the pre-commit hook does not support PHP7 return type declarations.

[netweb]

The 43218.4.diff​ adds the following updates from 43218.3.diff​ :

• Replaces spaced indentation sections of phpunit.xml.dist with tabs
• Removes PHPUnit --verbose flag that caused the PHPUnit Grunt task to fail, see Travis CI build ​#208
• Updates Travis CI config to use PHPUnit 7.x for PHP 7.1, 7.2, nightly PHP versions

Patch 43218.4.diff​ PR test: ​https://github.com/ntwb/wordpress/pull/17
Patch 43218.4.diff​ CI test: ​https://travis-ci.org/ntwb/wordpress/builds/352166794

p.s. It would be nice if the SVN history could be maintained by using some svn cp but I've not looked at how easily this could be achieved

[SergeyBiryukov]

Replying to netweb:

p.s. It would be nice if the SVN history could be maintained by using some svn cp but I've not looked at how easily this could be achieved

It can only be done on commit, I'll handle it once ​#meta3506 is resolved.

[SergeyBiryukov]

Replying to netweb:

Removes PHPUnit --verbose flag that caused the PHPUnit Grunt task to fail, see Travis CI build ​#208

It doesn't look the fail was due to the --verbose flag, but rather due to the duplicated phpunit7/testcase.php:
​https://github.com/ntwb/wordpress/pull/17/commits/dc563d50c564aad3a07f92efea4b39ff4a6a4534
​https://github.com/ntwb/wordpress/pull/17/commits/7d86d630fd68892da593f58a66d7ba535ddc7dc1

Or am I missing something?

[SergeyBiryukov]

Replying to netweb:

Removes PHPUnit --verbose flag that caused the PHPUnit Grunt task to fail, see Travis CI build ​#208

Ah, I see it in Travis CI build ​#207 now.

[netweb]

Replying to SergeyBiryukov:

... due to the duplicated phpunit7/testcase.php:

This was super annoying to get the patch right, SVN kept on duping the patch for these new files :(

[netweb]

Related: #43558 Add composer.json file for development dependencies

[pento]

@netweb: 43218.4.diff​ doesn't apply cleanly to trunk. Could you refresh it, please?

Also, a list of the svn cp commands to run would be helpful.

[netweb]

Note: Also update ​https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/ once merged

[desrosj]

43218.diff​ is a refreshed patch. It produces two weird failures, though. Investigating further, but it's possible I missed something in my refresh.

On PHPUnit 7:

1) WP_Test_REST_Schema_Sanitization::test_type_string
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.10'
+'1.1'
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/rest-api/rest-schema-sanitization.php:41

On PHPUnit 6:

1) Tests_Multisite_Site_Meta::test_site_meta_should_be_deleted_when_site_is_deleted
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'bar'
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/multisite/siteMeta.php:218

[SergeyBiryukov]

43218.diff​ is missing some later changes to phpunit/includes/testcase.php: [42836], [43005], [44327], [44633].

Working on fixing this and getting the patch committed.

[SergeyBiryukov]

In 44701:

Build/Test Tools: Add support for PHPUnit 7.x.

• Create an abstract WP_UnitTestCase_Base class to share between PHPUnit 7.x and older versions.
• Add a speed-trap loader to determine which SpeedTrapListener class needs to be loaded for the current PHPUnit version.
• Remove unnecessary PHPUnit\Util\Test and PHPUnit_Util_Getopt inheritances.
• Update Travis CI config to use PHPUnit 7.x for PHP 7.1, 7.2, and nightly PHP versions.

Props jipmoors, netweb, desrosj, ayeshrajans, soulseekah, SergeyBiryukov.
See #43218.

[SergeyBiryukov]

Replying to desrosj:

On PHPUnit 7:

1) WP_Test_REST_Schema_Sanitization::test_type_string
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.10'
+'1.1'
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/rest-api/rest-schema-sanitization.php:41

This test passes on PHPUnit 7.1 and fails on 7.2+, investigating further.

Also seeing some warnings about phpunit.xml.dist with 7.2+:

  Warning - The configuration file did not pass validation!
  The following problems have been detected:

  Line 17:
  - Element 'file': This element is not expected. Expected is ( exclude ).

  Line 37:
  - Element 'log', attribute 'logIncompleteSkipped': The attribute 'logIncompleteSkipped' is not allowed.

  Line 78:
  - Element 'directory': This element is not expected. Expected is ( file ).

  Test results may not be as expected.

[SergeyBiryukov]

Apparently the logIncompleteSkipped option was removed in PHPUnit 6.0:

• ​https://github.com/sebastianbergmann/phpunit/issues/2779
• ​https://github.com/sebastianbergmann/phpunit-documentation/issues/463

[SergeyBiryukov]

In 44702:

Build/Test Tools: Reorder <file> and <exclude> rules in phpunit.xml.dist to avoid warnings in PHPUnit 7.2+.

Remove deprecated logIncompleteSkipped option that does not exist in PHPUnit 6.0+.

See #43218.

[SergeyBiryukov]

Replying to desrosj:

On PHPUnit 7:

1) WP_Test_REST_Schema_Sanitization::test_type_string
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.10'
+'1.1'
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/rest-api/rest-schema-sanitization.php:41

This appears to be an error in the test. The value actually passed to rest_sanitize_value_from_schema() in this case is float(1.1), and the output is string(3) "1.1", as expected.

In PHPUnit 7.1 and older versions, this passes:

$this->assertEquals( '1.10', '1.1' );

In PHPUnit 7.2+, it does not.

We can fix the test by checking for both 1.1 float and '1.10' string explicitly.

[SergeyBiryukov]

In 44703:

Build/Test Tools: Correct WP_Test_REST_Schema_Sanitization::test_type_string() to check for both 1.1 float and '1.10' string explicitly.

Previously, the test only passed due to a bug in PHPUnit 7.1.x and older versions.

Fixes #43218. See #38586.

[SergeyBiryukov]

In 44704:

Build/Test Tools: Reorder <file> and <exclude> rules in multisite.xml to avoid warnings in PHPUnit 7.2+.

Missed in [44702].

See #43218.

[SergeyBiryukov]

In 44705:

Coding Standards: Replace spaced indentation sections of phpunit.xml.dist, multisite.xml, and build.xml with tabs.

Props netweb, desrosj.
See #43218.

[johnbillion]

This appears to have broken the ability to use any long option except for --exclude-group, --group, and --verbose. For example, --filter no longer works.

[johnbillion]

Clarification: Using other long options results in a fatal with the following error:

Fatal error: Uncaught Exception: unrecognized option --filter in tests/phpunit/includes/bootstrap.php on line 278

[SergeyBiryukov]

In 44707:

Build/Test Tools: Don't throw an exception for unrecognized options in WP_PHPUnit_Util_Getopt::parseLongOption().

The method is only used for two known options: group and exclude-group, others should be passed to PHPUnit.

Props johnbillion.
See #43218.

[SergeyBiryukov]

Replying to johnbillion:

This appears to have broken the ability to use any long option except for --exclude-group, --group, and --verbose. For example, --filter no longer works.

Thanks for catching that! [44707] fixes it, but we can probably remove the copied parseLongOption() method altogether, it seems like an overkill for just two options.

Previously we called the method from the parent PHPUnit_Util_Getopt class, but that's no longer possible, as the class is declared as final in PHPUnit 7.x.

[SergeyBiryukov]

In 44715:

Build/Test Tools: Simplify WP_PHPUnit_Util_Getopt and update documentation.

Introduced in [918/tests] and updated in [26871], the class is there to make sure that known bugs are not skipped when phpunit is called with a --group that corresponds to an @ticket annotation (such as phpunit --group 12345).

Enforcing recognized arguments or correctly formed arguments is not the concern here, so reusing PHPUnit_Util_Getopt::parseLongOption() was superfluous.

See #43218.

[SergeyBiryukov]

For some reason, using a @ticket annotation as a --group parameter (e.g. phpunit --group 21117) works in PHPUnit 7.1 and older versions, but not in PHPUnit 7.2+.

Per the ​documenation, @ticket is supposed to be an alias for @group, but apparently something has changed.

Investigating further. [44715] did not affect that.

[SergeyBiryukov]

Replying to SergeyBiryukov:

For some reason, using a @ticket annotation as a --group parameter (e.g. phpunit --group 21117) works in PHPUnit 7.1 and older versions, but not in PHPUnit 7.2+.

Not specific to @ticket, numeric groups don't work in PHPUnit 7.2+ either. If a test is marked as:

/**
 * @ticket 21117
 * @group 21117
 * @group test-group
 */

only test-group works.

phpunit list-groups does include 21117 though.

[SergeyBiryukov]

In 44719:

Build/Test Tools: Move WP_UnitTestCase_Base::assertPostConditions() to a more appropriate place.

See #43218.

[SergeyBiryukov]

Replying to SergeyBiryukov:

For some reason, using a @ticket annotation as a --group parameter (e.g. phpunit --group 21117) works in PHPUnit 7.1 and older versions, but not in PHPUnit 7.2+.

Created an upstream issue for this: ​https://github.com/sebastianbergmann/phpunit/issues/3502

If we find a workaround that could be implemented on our end, let's create a new ticket.

Also created #46149 for PHPUnit 8.x support.

[SergeyBiryukov]

With PHPUnit 7.x support added, we should update the ​Setup section in the handbook at some point, but I'd like to wait until ​https://github.com/sebastianbergmann/phpunit/issues/3502 is resolved before doing that.

[SergeyBiryukov]

Replying to SergeyBiryukov:

With PHPUnit 7.x support added, we should update the ​Setup section in the handbook at some point

The issue with numeric --group parameter is fixed in PHPUnit 7.5.4. Updated the handbook to mention that the latest PHPUnit 7.x release can now be used.