Skip to content

Conversation

@grogy
Copy link
Member

@grogy grogy commented Feb 15, 2022

@grogy grogy requested a review from jrfnl February 15, 2022 13:53
@grogy grogy marked this pull request as ready for review February 15, 2022 13:53
Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

This code base contains PHP version specific code. To properly record and manage code coverage, there should be (at least) two code coverage builds, one against a low PHP version, one against a high PHP version, so I'd like to propose to run code coverage against PHP 5.3 and PHP 8.1, instead of against PHP 7.4.

Note: to implement this, see the example code I linked to in the earlier issue as this will need the COVERALLS_PARALLEL env and the coveralls-finish job, as well as a PHP version switch to a higher PHP version (for the PHP 5.3 run) for sending the coverage reports to Coveralls.

@jrfnl jrfnl added this to the 1.0.0 milestone Feb 15, 2022
@grogy grogy force-pushed the coveralls branch 10 times, most recently from a91885f to cee7c96 Compare February 16, 2022 14:34
@grogy
Copy link
Member Author

grogy commented Feb 16, 2022

Thank you, I missed different code for php versions.

I am add parallel coverage for 5.3 and 8.1. Iteresting part is using composer global install for library. The reason is compatibility with PHP 5.3 and installed dependencies.

Example of coverage is https://coveralls.io/builds/46603511

Edit: in test section I let all PHP versions. There is running PHP unit, but also PHPLint.

@grogy grogy requested a review from jrfnl February 16, 2022 14:40
@grogy
Copy link
Member Author

grogy commented Feb 17, 2022

Please, @jrfnl, look to changes :-)

@jrfnl
Copy link
Collaborator

jrfnl commented Feb 17, 2022

in test section I let all PHP versions. There is running PHP unit, but also PHPLint.

I wouldn't worry about the linting for the PHP versions in the coverage run as if the code doesn't lint, the tests will fail anyway as 100% of the code is touched by the test run.

If needs be, the lint step (only one step after all) can be duplicated in the coverage workflow, which is better than running two test builds for the same PHP versions.

Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@grogy Nearly there!

Nice solution using the composer global require for the Coveralls package. 👍🏻
Makes sense to me and as the command is only for CI anyway, there is no pertinent reason to use a local install.

When the test run duplication is being resolved (see previous comment) and PHP 5.3 is no longer run in the test job, the "Setup ini config" step can be removed from the test job as it is then only needed in the coverage job.
Same for the "Composer: remove PHPCS" step, which is also only needed for the PHP 5.3 build.

Last note: the coveralls-finish job has a condition to prevent running on forks, but the coverage job does not. Either both should have that condition or neither.

@jrfnl jrfnl mentioned this pull request Feb 18, 2022
@grogy grogy force-pushed the coveralls branch 2 times, most recently from 341773d to 392c7a3 Compare February 18, 2022 08:39
@grogy
Copy link
Member Author

grogy commented Feb 18, 2022

Thank you @jrfnl , again you have interesting and correct points. 👍

  • PHP 5.3 and 8.1 is now tested and linted in coverage section
  • condition for prevent running on forks is in coverage section too
  • from test section are removed PHP 5.3 specialities

@jrfnl jrfnl modified the milestones: 1.0.0, 1.x Next Release Feb 18, 2022
Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

🎉

@jrfnl jrfnl merged commit d18311e into master Feb 18, 2022
@jrfnl jrfnl deleted the coveralls branch February 18, 2022 12:36
@jrfnl
Copy link
Collaborator

jrfnl commented Feb 19, 2022

@grogy Want to do a call again to plan out how we'll get the main Parallel Lint repo sorted as well ? If so, when would suit you ?
Also: is there a way to plan calls and such via private messages instead ? Like, are you on Twitter or in a particular Slack where we could plan this ? (or maybe this could be done via a maintainers team here on GH ?)

@grogy
Copy link
Member Author

grogy commented Feb 21, 2022

@jrfnl I will send you a messenge on Twitter this week ;)

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

Development

Successfully merging this pull request may close these issues.

Add support for Coveralls.io

2 participants