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 #3395

Closed
jrfnl opened this issue Jul 23, 2021 · 4 comments
Closed

Refactor/modernize test suite setup #3395

jrfnl opened this issue Jul 23, 2021 · 4 comments

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jul 23, 2021

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.

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 6, 2021

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.

@jrfnl jrfnl changed the title CI: Tests can no longer run on PHP 8.1 with PHPUnit 7.x Refactor/modernize test suite setup Sep 6, 2021
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 19, 2023

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 probabbly 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.

@gsherwood
Copy link
Member

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.

I don't think it's a big deal. I used it during the build of PHPCBF to see how I was going with auto-fixing, but I don't look at it any more. I'm happy to remove it and then look to adding it back in a different way if we ever feel it's needed again.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 2, 2023

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#25

@jrfnl jrfnl closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants