Tests: Improve PHPUnit Forward Compatibility #8607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swissspidy
added
Type: Infrastructure
This comment has been minimized.
This comment has been minimized.
swissspidy
changed the title
Closed
> PHPUnit 8.0.0 introduced a `void` return type declaration to the "fixture" methods – `setUpBeforeClass()`, `setUp()`, `tearDown()` and `tearDownAfterClass()`. As the `void` return type was not introduced until PHP 7.1, this makes it more difficult to create cross-version compatible tests when using fixtures, due to signature mismatches. > > The `Yoast\PHPUnitPolyfills\TestCases\TestCase` overcomes the signature mismatch by having two versions. The correct one will be loaded depending on the PHPUnit version being used. > > When using this TestCase, if an individual test, or another TestCase which extends this TestCase, needs to overload any of the "fixture" methods, it should do so by using a snake_case variant of the original fixture method name, i.e. `set_up_before_class()`, `set_up()`, `assert_pre_conditions()`, `assert_post_conditions()`, `tear_down()`, and `tear_down_after_class()`. > > The snake_case methods will automatically be called by PHPUnit. > > > IMPORTANT: The snake_case methods should not call the PHPUnit parent, i.e. do not use `parent::setUp()` from within an overloaded `set_up()` method. If necessary, DO call `parent::set_up()`. Reference: Yoast/PHPUnit-Polyfills#testcases This commit renames all declared fixture methods, and calls to parent versions of those fixture methods, from camelCase to snake_case.
The `assertRegExp()` and `assertNotRegExp()` methods were hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0. The `assertMatchesRegularExpression()` and `assertDoesNotMatchRegularExpression()` methods were introduced as a replacement in PHPUnit 9.1. These new PHPUnit methods are polyfilled by the PHPUnit Polyfills and switching to them will future-proof the tests some more. References: * sebastianbergmann/[email protected]/ChangeLog-9.1.md#910---2020-04-03 * sebastianbergmann/phpunit#4085 * sebastianbergmann/phpunit#4088
Since PHPUnit 8, PHPUnit has a built-in caching feature which creates a `.phpunit.result.cache` file. This file should not be committed.
… when used with strings. Using the `assertContains()` and `assertNotContains()` methods with string haystacks was deprecated in PHPUnit 8 and removed in PHPUnit 9. These methods introduced in PHPUnit 7.5 should be used as an alternative: * `assertStringContainsString()` * `assertStringContainsStringIgnoringCase()` * `assertStringNotContainsString()` * `assertStringNotContainsStringIgnoringCase()`
The assertInternalType() and assertNotInternalType() methods are deprecated in PHPUnit 8 and removed in PHPUnit 9. These methods introduced in PHPUnit 7.5 should be used as an alternative: * `assertIsArray()` * `assertIsBool()` * `assertIsFloat()` * `assertIsInt()` * `assertIsNumeric()` * `assertIsObject()` * `assertIsResource()` * `assertIsString()` * `assertIsScalar()` * `assertIsCallable()` * `assertIsIterable()` * `assertIsNotArray()` * `assertIsNotBool()` * `assertIsNotFloat()` * `assertIsNotInt()` * `assertIsNotNumeric()` * `assertIsNotObject()` * `assertIsNotResource()` * `assertIsNotString()` * `assertIsNotScalar()` * `assertIsNotCallable()` * `assertIsNotIterable()`
The current config files validate against the PHPUnit XSD schema for config files for PHPUnit 5.7 – 9.2. The schema was changed in PHPUnit 9.3, and the `filter` and `logging` settings were deprecated in favor of `coverage` and a different format for `logging`. This commit explicitly sets the schema against which the files currently validate, for clarity.
Defines this new constant as proposed in WordPress/wordpress-develop#1563
swissspidy
force-pushed
the
fix/phpunit-compat
branch
from
6bdbddf to
8431556
Compare
`_delete_all_data()` in the WP test suite will delete all users anyway.
8 tasks
swissspidy
changed the title
swissspidy
changed the title
swissspidy
changed the title
Comment on lines
-42
to
-44
| public static function wpTearDownAfterClass() { | ||
| self::delete_user( self::$user_id ); | ||
| } |
There was a problem hiding this comment.
WordPress already deletes all users in \WP_UnitTestCase_Base::tear_down_after_class
So this is redundant.
| $experiments->method( 'is_experiment_enabled' ) | ||
| ->willReturn( true ); | ||
|
|
||
| $this->instance = new \Google\Web_Stories\Media\SVG( $experiments ); |
There was a problem hiding this comment.
I don't like this. It is much cleaner to create a new instance of a object every time for each test. So there no problem of state being from previous test. The test was better the way it was.
There was a problem hiding this comment.
I was doing some cleanup like this while working on this PR to make things more DRY.
Note that this is in set_up so we still get a new instance for every test. No need to repeat this when set_up can do this once for every test.
So I disagree with the "The test was better the way it was"
|
This PR needs to be rebased. There seems to be lots of places where you have converted to use a single instance of the class. This bad for unit testing and should be reverted. What was the reasoning behind this change in the first place? Also, all the instance where users can deleted after tests are deleted, why were these removed? |
| uses: shivammathur/setup-php@v2 | ||
| with: | ||
| php-version: 7.4 | ||
| php-version: '8.0' |
There was a problem hiding this comment.
Does this need quote marks?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PHPUnit 8.x and 9.x compatibility
tl;dr: we need to update our PHP integration tests running against core so that they function properly (again) as needed.
WordPress core has moved to adopt PHPUnit polyfills for running its tests and at the same time runs tests using the latest possible PHPUnit version.
To make our test suite compatible, we need to:
WP_TESTS_PHPUNIT_POLYFILLS_PATHYoast/wp-test-utilspackageThe PR also includes one small change related to PHP 8.1 compatibility, see #8520.
See https://core.trac.wordpress.org/ticket/46149 and some of the early individual commit messages for details.
See also https://core.trac.wordpress.org/ticket/53911 for when the PHPUnit changes were backported to older branches