squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

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

Closed jrfnl closed 9 months ago

jrfnl commented 1 year ago

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.

GetMemberPropertiesTest ++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 two ++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().

RuleInclusionTest ++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:

Related to (but doesn't fix) #3395

Fixes #3490

jrfnl commented 1 year ago

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

jrfnl commented 1 year ago

Rebased without changes to fix merge conflict after merge of #3708.

jrfnl commented 1 year ago

I've rebased this PR and updated it to incorporate the changes needed in the tests introduced by today's merges. Updated the PR description to match.

Note: there will now 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).

jrfnl commented 10 months ago

Note: depending on which PR gets merged first, this PR will need to update the CONTRIBUTING.md file and the Composer scripts which PR #3830 add.

jrfnl commented 9 months ago

Closing as replaced by https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/59