WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 7 weeks ago

#43218 accepted defect (bug)

PHPUnit 7.x support

Reported by: ayeshrajans Owned by: SergeyBiryukov
Milestone: 5.0 Priority: normal
Severity: normal Version: trunk
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description

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.

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.

Attachments (4)

Provide_PHPUnit_7_compatibility.patch (79.4 KB) - added by jipmoors 3 months ago.
Implementing PHPUnit 7 compatibility
43218.2.diff (77.7 KB) - added by netweb 3 months ago.
43218.3.diff (95.5 KB) - added by SergeyBiryukov 3 months ago.
43218.4.diff (87.7 KB) - added by netweb 3 months ago.

Download all attachments as: .zip

Change History (27)

#1 @jrf
4 months ago

  • Focuses coding-standards removed

#2 @jorbin
4 months ago

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 ?

#3 @ayeshrajans
4 months ago

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.

#4 @ayeshrajans
4 months ago

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.

This ticket was mentioned in Slack in #docs by atachibana. View the logs.


3 months ago

#6 @soulseekah
3 months ago

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.

#7 follow-up: @dingo_bastard
3 months ago

  • Keywords needs-patch added

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?

#8 in reply to: ↑ 7 @soulseekah
3 months ago

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

#9 @dingo_bastard
3 months ago

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

@jipmoors
3 months ago

Implementing PHPUnit 7 compatibility

#10 @jipmoors
3 months ago

  • Keywords has-patch added; needs-patch removed

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.

#11 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.0

#12 @SergeyBiryukov
3 months ago

In 42799:

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

Props jipmoors.
See #30988, #43218.

@netweb
3 months ago

#13 follow-up: @netweb
3 months ago

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

#14 in reply to: ↑ 13 ; follow-up: @netweb
3 months ago

Testing 43218.2.diff in Travis CI

#15 in reply to: ↑ 14 @netweb
3 months ago

Replying to netweb:

Testing 43218.2.diff in Travis CI

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

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

#16 @SergeyBiryukov
3 months ago

  • Keywords commit added
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

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.

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


3 months ago

@netweb
3 months ago

#18 follow-ups: @netweb
3 months ago

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

#19 in reply to: ↑ 18 @SergeyBiryukov
2 months ago

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.

#20 in reply to: ↑ 18 ; follow-up: @SergeyBiryukov
2 months ago

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?

#21 in reply to: ↑ 18 @SergeyBiryukov
2 months ago

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.

#22 in reply to: ↑ 20 @netweb
2 months ago

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

#23 @netweb
7 weeks ago

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

Note: See TracTickets for help on using tickets.