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

Run tests on PHPUnit 9, on PHP 7.4 and add .gitattributes to exclude dev files from exports #46

Merged
merged 3 commits into from
Aug 14, 2020

Conversation

SimonFrings
Copy link
Contributor

No description provided.

@clue clue added this to the v1.0.0 milestone Aug 10, 2020
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonFrings Thanks for the update!

It looks like this currently reports a warning, is this something you can look into?

https://travis-ci.org/github/graphp/graphviz/jobs/716530963#L526

It also looks like this creates a .phpunit.result.cache file when executed. Can we prevent or ignore this file?

@SimonFrings
Copy link
Contributor Author

@SimonFrings Thanks for the update!

It looks like this currently reports a warning, is this something you can look into?

https://travis-ci.org/github/graphp/graphviz/jobs/716530963#L526

It also looks like this creates a .phpunit.result.cache file when executed. Can we prevent or ignore this file?

PHPUnit 9.3 released a new schema for the phpunit.xml configuration file. I had to migrate the file to the new format in order to avoid the warning. That also means every PHPUnit Version older than 9.3 will automatically report a warning when executing the test suite (because the new format is unknown).

It also appears that the <phpunit cacheResult="true|false"> XML configuration attribute was set to true instead of false as default. That caused the .phpunit.result.cache to be generated.

@clue
Copy link
Member

clue commented Aug 11, 2020

@SimonFrings Thanks for the update! It looks like the new configuration file format is only supported by PHPUnit 9.3+, so older PHPUnit versions (on older PHP versions) will no longer generate a proper code coverage output.

This means we can not use a single configuration file that works across all PHPUnit versions referenced in this package. We can either ignore the warnings on some versions or ship multiple configuration files and configure our test jobs accordingly. Given PHPUnit's track record of deprecating and removing legacy functionality, there's a fair chance ignoring any warnings on newer versions isn't sustainable in the long run.

The result cache is kind of pointless for small libraries such as this one. I agree that turning it off is probably the best option for now. It's interesting old PHPUnit versions do not seem to complain about this (unknown) option. https://github.com/sebastianbergmann/phpunit/blob/8c908255db023c3861a8f3adc2cad1ed61dc4690/ChangeLog-9.3.md#934---2020-08-10 leads to sebastianbergmann/phpunit@a89b9ae, so it looks like this was supposed to be enabled as of PHPUnit 8+ but somewhat was disabled between PHPUnit 9 and PHPUnit 9.3.3.

@SimonFrings
Copy link
Contributor Author

I went with the possibility to ship multiple configuration files. I told .travis.yml to use the phpunit.xml.dist for PHPUnit 9.3+ and the phpunit-legacy.xml.dist for older PHPUnit Versions.

@SimonFrings SimonFrings changed the title Run tests on PHPUnit 9, on PHP7.4 and add .gitattributes to exclude dev files from exports Run tests on PHPUnit 9, update PHPUnit configuration schema and add .gitattributes to exclude dev files from exports Aug 14, 2020
@SimonFrings SimonFrings changed the title Run tests on PHPUnit 9, update PHPUnit configuration schema and add .gitattributes to exclude dev files from exports Run tests on PHPUnit 9, on PHP 7.4 and add .gitattributes to exclude dev files from exports Aug 14, 2020
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonFrings Thanks for the update, changes LGTM! :shipit:

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

Successfully merging this pull request may close these issues.

2 participants