Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: allow the test suite to run on PHPUnit 8.x and 9.x #59

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 10, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3803:

GetMethodParametersTest: add some missing array indexes expectations

This commit:

  1. Fixes the order of a few array entries.
  2. Adds some array entries which were supposed to be expected, but missing.

The use of assertArraySubset() hid the fact that these entries were missing. As that assertion now needs to be replaced, these issues came to light.

GetMemberPropertiesTest: add some missing array indexes expectations

This commit adds some array entries which were supposed to be expected, but missing.

The use of assertArraySubset() hid the fact that these entries were missing. As that assertion now needs to be replaced, these issues came to light.

File/Get*Tests: work round removal of assertArraySubset()

The assertArraySubset() method was deprecated in PHPUnit 8.x and removed in PHPUnit 9.0.0 without replacement.

The assertArraySubset() assertion was being used as not all token array indexes are being tested - to be specific: any index which is a token offset is not tested -.
As the assertArraySubset() has been removed, I'm electing to unset the token offset array indexes and replacing the assertion with a strict type assertSame() comparison.

File/Get*Tests:: work round removal of exception related annotations

Expecting exceptions via annotations was deprecated in PHPUnit 8.x and removed in PHPUnit 9.0.0 in favour of using the expectException*() methods.

This does need a work around for PHPUnit 4.x in which the expectException*() methods didn't exist yet, but as this only applies to three tests, that's not a big deal.

GotoLabelTest: work round removal of assertInternalType()

The assertInternalType() method was deprecated in PHPUnit 7.5.0 and removed in PHPUnit 9.0.0.
PHPUnit 7.5.0 introduced dedicated assertIs*() (like assertIsInt()) methods as a replacement.

As this is only a simple check in these two tests, a PHPUnit feature based toggle seems over the top, so I'm just replacing the assertion with an alternative which will work PHPUnit cross-version.

RuleInclusion*Test: remove a few redundant assertions

These assertions are checking whether explicitly declared properties exist, which is redundant.

Removing the assertions does not diminish the value of the tests as there are follow-up assertions testing the value of the properties.

Removing the assertions also gets rid of a warning thrown in PHPUnit 9.6.x about the assertObjectHasAttribute() assertion being removed in PHPUnit 10.0.
Note: PHPUnit 10.1.0 adds these assertions back again, but under a different name assertObjectHasProperty().

Ruleset Tests: work round removal of assertObjectHasAttribute()

The assertObjectHasAttribute() method was deprecated in PHPUnit 9.6.x and removed in PHPUnit 10.0.0 without replacement.
Note: PHPUnit 10.1.0 adds the assertion back again, but under a different name assertObjectHasProperty().

While only a deprecation warning is shown on PHPUnit 9.6.x and the tests will still pass, I'm electing to replace the assertion anyway with code which emulates what PHPUnit would assert.

Tests: rename fixture methods and use annotations

As of PHPUnit 8.x, the method signature for the setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass() fixture methods has changed to require the void return type.

As the void return type isn't available until PHP 7.1, this cannot be implemented.

Annotations to the rescue.
By renaming the setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass() methods to another, descriptive name and using the @beforeClass, @before, @after and @afterClass annotations, the tests can be made cross-version compatible up to PHPUnit 9.x.

With this change, the unit tests can now be run on PHPUnit 4 - 9.

This constitutes a minor BC-break for external standards which a) extend the PHPCS native testsuite and b) overload the AbstractSniffUnitTest::setUp() method.
While quite a few external standards extends the PHPCS native testsuite, I very much doubt any of these overload the setUp() method, so IMO and taking into account that this is a test-only change, this is an acceptable change to include in the next PHPCS minor.

Ref: https://docs.phpunit.de/en/7.5/annotations.html#before

Tests: allow the test suite to run on PHPUnit 8.x and 9.x

Includes:

  • composer.json: widening the PHPUnit requirement to allow for PHPUnit 8.x and PHPUnit 9.x.
    Note: The recently released PHPUnit 10.x is not (yet) supported as it no longer supports the old-style test suite runner setup.
    It also would require for the abstract base test cases to be renamed as those classes are no longer allowed to end on Test.
    Refactoring the test suite to allow for PHPUnit 10.x is for a future PR.
  • composer.json: remove the script which was specific for PHP 8.1+
  • Adjusting the PHPUnit configuration to ensure the tests are run in the same way and show all notices/warnings/deprecations on all PHPUnit versions.
    The default value for a number of configuration options has changed over time.
    This makes sure they are consistently set to values which are sensible for this codebase, independently of the PHPUnit version on which the tests are run.
    Includes adding a schema annotation (set to PHPUnit 9.2 as the schema has changed in PHPUnit 9.3, though that won't prevent the tests from running correctly).
  • GH Actions test workflow: removing work-arounds which were in place related to running PHPUnit 7.x on PHP 8.x.
  • AllTests: Adjusting the condition which determines which TestSuite file to load to allow for PHPUnit 8.x and 9.x.
  • Adding the .phpunit.result.cache file to .gitignore.
    PHPUnit has a caching feature build in as of PHPUnit 8, so ignore the file that generates to prevent it from being committed.
  • CONTRIBUTING: remove references to the remove Composer script + instructions which have now become redundant.

Related to (but doesn't fix) squizlabs/PHP_CodeSniffer#3395

Fixes squizlabs/PHP_CodeSniffer#3490


Additional notes from the original PR:

Note: this PR should not be ported to the 4.x branch as-is. Only select commits should be included in 4.x. If so preferred, I can set up a separate PR with the cherrypicked bits and pieces which should go into PHPCS 4.x.

Note: there will still be one warning on PHPUnit 9.x (which doesn't fail the build) due to the deprecation expectation. This is not something to worry about until PHPUnit 10.x would need to be supported (which is not needed yet as PHPUnit 9.x will run fine on PHP 8.2 and even 8.3).

Suggested changelog entry

  • Test suites for external standards which run via the PHPCS native test suite can now run on PHPUnit 4-9 (was 4-7).
    • If any of these tests use the PHPUnit setUp()/tearDown() methods or overload the one in the AbstractSniffUnitTest test case, they will need to be adjusted. See the PR details for further information.

Related issues/external references

Related to #25

This commit:
1. Fixes the order of a few array entries.
2. Adds some array entries which were supposed to be expected, but missing.

The use of `assertArraySubset()` hid the fact that these entries were missing. As that assertion now needs to be replaced, these issues came to light.
This commit adds some array entries which were supposed to be expected, but missing.

The use of `assertArraySubset()` hid the fact that these entries were missing. As that assertion now needs to be replaced, these issues came to light.
The `assertArraySubset()` method was deprecated in PHPUnit 8.x and removed in PHPUnit 9.0.0 without replacement.

The `assertArraySubset()` assertion was being used as not all token array indexes are being tested - to be specific: any index which is a token offset is not tested -.
As the `assertArraySubset()` has been removed, I'm electing to unset the token offset array indexes and replacing the assertion with a strict type `assertSame()` comparison.
Expecting exceptions via annotations was deprecated in PHPUnit 8.x and removed in PHPUnit 9.0.0 in favour of using the `expectException*()` methods.

This does need a work around for PHPUnit 4.x in which the `expectException*()` methods didn't exist yet, but as this only applies to three tests, that's not a big deal.
The `assertInternalType()` method was deprecated in PHPUnit 7.5.0 and removed in PHPUnit 9.0.0.
PHPUnit 7.5.0 introduced dedicated `assertIs*()` (like `assertIsInt()`) methods as a replacement.

As this is only a simple check in these two tests, a PHPUnit feature based toggle seems over the top, so I'm just replacing the assertion with an alternative which will work PHPUnit cross-version.
These assertions are checking whether explicitly declared properties exist, which is redundant.

Removing the assertions does not diminish the value of the tests as there are follow-up assertions testing the value of the properties.

Removing the assertions also gets rid of a warning thrown in PHPUnit 9.6.x about the `assertObjectHasAttribute()` assertion being removed in PHPUnit 10.0.
Note: PHPUnit 10.1.0 adds these assertions back again, but under a different name `assertObjectHasProperty()`.
The `assertObjectHasAttribute()` method was deprecated in PHPUnit 9.6.x and removed in PHPUnit 10.0.0 without replacement.
Note: PHPUnit 10.1.0 adds the assertion back again, but under a different name `assertObjectHasProperty()`.

While only a deprecation warning is shown on PHPUnit 9.6.x and the tests will still pass, I'm electing to replace the assertion anyway with code which emulates what PHPUnit would assert.
As of PHPUnit 8.x, the method signature for the `setUpBeforeClass()`, `setUp()`, `tearDown()` and `tearDownAfterClass()` fixture methods has changed to require the `void` return type.

As the `void` return type isn't available until PHP 7.1, this cannot be implemented.

Annotations to the rescue.
By renaming the `setUpBeforeClass()`, `setUp()`, `tearDown()` and `tearDownAfterClass()` methods to another, descriptive name and using the `@beforeClass`, `@before`, `@after` and `@afterClass` annotations, the tests can be made cross-version compatible up to PHPUnit 9.x.

With this change, the unit tests can now be run on PHPUnit 4 - 9.

This constitutes a minor BC-break for external standards which a) extend the PHPCS native testsuite and b) overload the `AbstractSniffUnitTest::setUp()` method.
While quite a few external standards extends the PHPCS native testsuite, I very much doubt any of these overload the `setUp()` method, so IMO and taking into account that this is a test-only change, this is an acceptable change to include in the next PHPCS minor.

Ref: https://docs.phpunit.de/en/7.5/annotations.html#before
@jrfnl jrfnl force-pushed the U-3803/feature/allow-for-phpunit-8-9 branch from 3ee3cc8 to fae662d Compare December 4, 2023 09:01
Includes:
* `composer.json`: widening the PHPUnit requirement to allow for PHPUnit 8.x and PHPUnit 9.x.
    Note: The recently released PHPUnit 10.x is not (yet) supported as it no longer supports the old-style test suite runner setup.
    It also would require for the abstract base test cases to be renamed as those classes are no longer allowed to end on `Test`.
    Refactoring the test suite to allow for PHPUnit 10.x is for a future PR.
* `composer.json`: remove the script which was specific for PHP 8.1+
* Adjusting the PHPUnit configuration to ensure the tests are run in the same way and show all notices/warnings/deprecations on all PHPUnit versions.
    The default value for a number of configuration options has changed over time.
    This makes sure they are consistently set to values which are sensible for this codebase, independently of the PHPUnit version on which the tests are run.
    Includes adding a schema annotation (set to PHPUnit 9.2 as the schema has changed in PHPUnit 9.3, though that won't prevent the tests from running correctly).
* GH Actions `test`/`quicktest` workflow: removing work-arounds which were in place related to running PHPUnit 7.x on PHP 8.x.
* GH Actions `phpstan` workflow: let PHPStan run on the latest PHP version, now there's no need anymore to run it against a lower PHP version to prevent deprecation notices related to the use of an outdated PHPUnit version.
* `AllTests`: Adjusting the condition which determines which `TestSuite` file to load to allow for PHPUnit 8.x and 9.x.
* Adding the `.phpunit.result.cache` file to `.gitignore`.
    PHPUnit has a caching feature build in as of PHPUnit 8, so ignore the file that generates to prevent it from being committed.
* CONTRIBUTING: remove references to the remove Composer script + instructions which have now become redundant.

Related to 3395
@jrfnl jrfnl force-pushed the U-3803/feature/allow-for-phpunit-8-9 branch from fae662d to 10b01fe Compare December 4, 2023 09:12
@jrfnl jrfnl merged commit 2418376 into master Dec 4, 2023
55 checks passed
@jrfnl jrfnl deleted the U-3803/feature/allow-for-phpunit-8-9 branch December 4, 2023 09:16
jrfnl added a commit to PHPCSStandards/PHPCSExtra that referenced this pull request Dec 8, 2023
PHP_CodeSniffer 3.8.0 now allows for running the tests, which are based on the PHPCS native test suite, with PHPUnit 8 and 9.

This commit updates the package to take advantage of that.

Includes:
* Widening the PHPUnit version requirements.
* Simplifications to the `quicktest` and `test` workflows.
    Also, the code coverage "high" run can now be run against PHP 8.3.
* Running PHPStan against PHP `latest` (couldn't previously be done due to the old PHPUnit version).

Not includes as this was already handled in this repo:
* Adding the PHPUnit cache file to `.gitignore`.
* Adding various PHPUnit config attributes to ensure deprecation notices and such still show on PHPUnit 9.

Ref:
* PHPCSStandards/PHP_CodeSniffer#59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with PhpUnit 9.5.10
1 participant