-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
PHPUnit tweaks #1628
PHPUnit tweaks #1628
Conversation
@jrfnl I'm sorry I'm not able to reproduce any bug. |
@kukulich I'll have a look later again later tonight and provide additional info. I also noticed (I should have known) that the PHPUnit config is causing build failures as PHPUnit 10.x no longer makes a distinction between PHP warnings and PHPUnit warnings and will fail the build on both. I'll make some tweaks to get that sorted later too. |
15f4f81
to
2adac04
Compare
@kukulich I've added an extra commit to upgrade the PHPUnit configuration to be valid against the PHPUnit 10.3+ schema and another commit to fix another CI issue (wrong Running the tests against PHP 8.2.8 with the PHPUnit 10.3.5 PHAR on Windows 10 shows me:
And when I then change the
I get the same results whether I run the tests via a local PHPUnit PHAR file, via the I made sure to run Not sure why you are not seeing the issues. Or why I am when I shouldn't ? I'm also mystified why these issues aren't showing in CI, not even with the additional commit turning On that note, might it be an idea to convert the PHING checks to Composer scripts ? |
The config as it was, was hiding 6 `Undefined array key`/`Trying to access array offset on null` notices. As any notice caused by a sniff will stop a PHPCS scan of a file dead, these should be discovered when running the tests and then fixed. Better yet: the tests should fail on them. This change in the configuration makes it so. Note: this does mean you now have a bug to fix. Also see: squizlabs/PHP_CodeSniffer#3844
This upgrades the XML configuration to one accepted by PHPUnit 10.3 and higher, which is in line with the PHPUnit requirements in the `composer.json` file. Note: the configuration will not validate for PHPUnit < 10.3 and will throw warnings, but as code coverage in CI is only run with PHPUnit 10 and the warnings are about that part of the configuration, this is nothing to worry about. Moreover, on PHPUnit 8.x/9.x, those warnings won't fail the build and the tests will still be run.
The default ini file used by the SetupPHP action is the production one, which turns error_reporting/display off. For the purposes of CI, I'd recommend running with `error_reporting=-1` and `display_errors=On` to ensure **all** PHP notices are shown. Also see: shivammathur/setup-php#469
PHPUnit 8.x has a minimum supported version of PHP 7.2, so there is no need to include PHPUnit 7.x in the allowed versions.
2adac04
to
cff76d6
Compare
Thanks. I will merge the PR now and try to fix the warnings later. |
.gitignore: ignore PHPUnit cache file
PHPUnit config: display errors/warnings/notices etc
The config as it was, was hiding 6
Undefined array key
/Trying to access array offset on null
notices.As any notice caused by a sniff will stop a PHPCS scan of a file dead, these should be discovered when running the tests and then fixed.
Better yet: the tests should fail on them. This change in the configuration makes it so.
Note: this does mean you now have a bug to fix.
Also see: squizlabs/PHP_CodeSniffer#3844
PHPUnit config: upgrade to 10.3 schema
This upgrades the XML configuration to one accepted by PHPUnit 10.3 and higher, which is in line with the PHPUnit requirements in the
composer.json
file.Note: the configuration will not validate for PHPUnit < 10.3 and will throw warnings, but as code coverage in CI is only run with PHPUnit 10 and the warnings are about that part of the configuration, this is nothing to worry about. Moreover, on PHPUnit 8.x/9.x, those warnings won't fail the build and the tests will still be run.
GH Actions: turn error reporting on
The default ini file used by the SetupPHP action is the production one, which turns error_reporting/display off.
For the purposes of CI, I'd recommend running with
error_reporting=-1
anddisplay_errors=On
to ensure all PHP notices are shown.Also see: shivammathur/setup-php#469
Composer: no need to allow PHPUnit 7.x
PHPUnit 8.x has a minimum supported version of PHP 7.2, so there is no need to include PHPUnit 7.x in the allowed versions.