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

Refactor/modernize test suite setup #25

Open
jrfnl opened this issue Nov 8, 2023 · 2 comments
Open

Refactor/modernize test suite setup #25

jrfnl opened this issue Nov 8, 2023 · 2 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Nov 8, 2023

Repost from squizlabs/PHP_CodeSniffer#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 squizlabs/PHP_CodeSniffer#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: Deprecate "Test" suffix for abstract test case classes sebastianbergmann/phpunit#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
Copy link
Member Author

jrfnl commented Dec 27, 2023

Also see: PHPCSStandards/PHPCSUtils#72

@jrfnl
Copy link
Member Author

jrfnl commented Feb 19, 2024

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:

  • The test setup has been simplified to run with TestSuites defined in the PHPUnit config file instead of setting up a custom TestSuite.
    This will also simplify running the tests for external standards, though they will need to make some changes. A rough draft of an upgrade guide for this has been prepared.
  • The printPHPCodeSnifferTestOutput() will be dropped for now. If people miss it terribly, it could be brought back via a TestListener/EventListener at a later point in time, but that doesn't currently have priority.
  • All uses of global variables in the test suite have been removed.
  • The abstract base test cases will be renamed to have a TestCase suffix.
  • The test suite for 4.0 will be compatible with PHPUnit ^8.0 || ^9.3.4 || ^10.1.0 || ^11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant