Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#43218 closed defect (bug) (fixed)

PHPUnit 7.x support

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Build/Test Tools Keywords: has-patch needs-docs needs-testing
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 (6)

Provide_PHPUnit_7_compatibility.patch (79.4 KB) - added by jipmoors 6 years ago.
Implementing PHPUnit 7 compatibility
43218.2.diff (77.7 KB) - added by netweb 6 years ago.
43218.3.diff (95.5 KB) - added by SergeyBiryukov 6 years ago.
43218.4.diff (87.7 KB) - added by netweb 6 years ago.
43218.diff (89.2 KB) - added by desrosj 5 years ago.
43218.5.diff (89.5 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (58)

#1 @jrf
6 years ago

  • Focuses coding-standards removed

#2 @jorbin
6 years 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
6 years 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
6 years 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.


6 years ago

#6 @soulseekah
6 years 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
6 years 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
6 years 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
6 years ago

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

@jipmoors
6 years ago

Implementing PHPUnit 7 compatibility

#10 @jipmoors
6 years 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
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#12 @SergeyBiryukov
6 years ago

In 42799:

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

Props jipmoors.
See #30988, #43218.

@netweb
6 years ago

#13 follow-up: @netweb
6 years 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
6 years ago

Testing 43218.2.diff in Travis CI

#15 in reply to: ↑ 14 @netweb
6 years 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
6 years 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.


6 years ago

@netweb
6 years ago

#18 follow-ups: @netweb
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years ago

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

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


6 years ago

#26 @jorbin
6 years ago

  • Milestone changed from 5.0 to 5.1

#27 @pento
5 years ago

  • Keywords needs-refresh added; commit removed

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

#28 @netweb
5 years ago

  • Keywords needs-docs added

@desrosj
5 years ago

#29 follow-ups: @desrosj
5 years ago

  • Keywords needs-testing added; needs-refresh removed

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

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


5 years ago

#31 @SergeyBiryukov
5 years ago

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.

#32 @SergeyBiryukov
5 years ago

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.

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


5 years ago

#34 in reply to: ↑ 29 @SergeyBiryukov
5 years ago

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.
Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#36 @SergeyBiryukov
5 years ago

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.

#37 in reply to: ↑ 29 @SergeyBiryukov
5 years ago

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.

#38 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#39 @SergeyBiryukov
5 years ago

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.

#40 @SergeyBiryukov
5 years ago

In 44705:

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

Props netweb, desrosj.
See #43218.

#41 follow-up: @johnbillion
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#42 @johnbillion
5 years ago

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

#43 @SergeyBiryukov
5 years ago

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.

#44 in reply to: ↑ 41 @SergeyBiryukov
5 years ago

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.

#45 @SergeyBiryukov
5 years ago

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.

#46 follow-ups: @SergeyBiryukov
5 years ago

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.

#47 in reply to: ↑ 46 @SergeyBiryukov
5 years ago

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.

#48 @SergeyBiryukov
5 years ago

In 44719:

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

See #43218.

#49 in reply to: ↑ 46 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#50 follow-up: @SergeyBiryukov
5 years ago

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.

#51 in reply to: ↑ 50 @SergeyBiryukov
5 years ago

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.

This ticket was mentioned in Slack in #cli by dingo_d. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.