search

PHPCSStandards / PHP_CodeSniffer

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

Refactor/modernize test suite setup #25

Open jrfnl opened 1 year ago

jrfnl commented 1 year ago

Repost from https://github.com/squizlabs/PHP_CodeSniffer/issues/3395:

As I've been seeing new issues popping up elsewhere with some of the recent changes in PHP 8.1, I had a look at the latest test run for this repo and as I feared, the tests break on PHP 8.1 due to PHPUnit 7.5 being used.

See: https://github.com/squizlabs/PHP_CodeSniffer/runs/3118862021?check_suite_focus=true

As PHPUnit 7.5 is no longer supported, this will not be fixed on the PHPUnit side of things.

I think a refactor of the test suite setup is in order to allow it to be compatible with a wider range of PHPUnit versions, preferably without BC-break in the setup as quite a lot of external standards use the PHPCS native abstract test classes for their tests as well.

Initial part solved via https://github.com/squizlabs/PHP_CodeSniffer/pull/3400

While the PHP 8.1 issues have been solved by now, I'm going to leave this issue open, as IMO a refactor of the test suite setup may still be a good idea and make the setup more sustainable in the long run.

The setup as is, with the AllTests, the dynamic test suite creation etc seems to be based on a really old PHPUnit version and while it still works, the framework as-is, is creaking more and more with each new PHP version.

As things are at this time, if PHPCS wants to allow running tests on PHPUnit 10.x (which it should), at least two things needs to happen (maybe more):

  1. Rename abstract base test cases. Those classes are no longer allowed to have the Test suffix. I'd recommend renaming the suffix from UnitTest to TestCase. See: https://github.com/sebastianbergmann/phpunit/issues/5132
  2. Changing the test suite set up to remove the use of the old style set up with AllTests.php and AllSniffs.php containing a static suite method, which is no longer supported.

    PHPUnit no longer invokes a static method named suite on a class that is declared in a file that is passed as an argument to the CLI test runner Ref: https://github.com/sebastianbergmann/phpunit/blob/10.0.19/ChangeLog-10.0.md#1000---2023-02-03 (Changed section) Also related:

If losing the line created via the bootstrap printPHPCodeSnifferTestOutput() method would not be considered a big deal, changing things round to remove the AllTests and AllSniffs files and letting PHPUnit self-discover the tests in a test suite would probably work. Includes and setting of globals and constants should probably be moved to the test bootstrap.php file.

The printPHPCodeSnifferTestOutput() output could possibly still be generated by creating a custom PHPUnit Extension (formally TestListener).

As fixing this would mean breaking changes for any external standard which uses the PHPCS base test classes for their own test suites, I'd recommend for these changes to be made in PHPCS 4.0.0 and no later than that.

jrfnl commented 11 months ago

Also see: https://github.com/PHPCSStandards/PHPCSUtils/issues/72

jrfnl commented 9 months ago

FYI: I've done all the prep for this for the 4.x branch over the last few days and managed to get things working.

The custom setup with the TestSuites appears to have been largely related/needed to support running the tests via a PEAR setup and needing to "unload" the PHPCS autoloader after setup in that scenario. As PEAR support has been dropped, this is no longer an issue.

Changes prepped for 4.0: