[TASK] Move the coverage generation to a separate CI workflow #427
Conversation
oliverklee
added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone
oliverklee
force-pushed
the
task/coverage-separate
branch
from
87a61db to
54aaec8
Compare
JakeQZ
force-pushed
the
task/coverage-separate
branch
from
54aaec8 to
6d96d8e
Compare
There was a problem hiding this comment.
That was an educational review. I understand now how the matrix works with include, but haven't quite got to grips with the with parameter. I don't understand what uses it and how. Maybe best keep those in for now.
I think you've added PHP 7.4 to the unit tests as well as code coverage, and would prefer to keep this PR to a refactoring only (with no overall change). I think a command line argument for code-coverage can now be dropped from the original unit tests section.
The number of CI tests should remain the same.
| php-version: ${{ matrix.php-version }} | ||
| ini-values: error_reporting=E_ALL | ||
| tools: composer:v2 | ||
| coverage: none |
There was a problem hiding this comment.
What uses this? Is it possible to lose the coverage: none?
There was a problem hiding this comment.
Setting coverage: none has the result of no code coverage tool (like Xdebug of PCOV) getting loaded, which should speed up the non-coverage tasks a bit.
oliverklee
force-pushed
the
task/coverage-separate
branch
2 times, most recently
from
345f50d to
171ea94
Compare
|
I have now restructured things a bit, moved the coverage generation to a separate workflow, and undone the cleanup/refactoring to keep the changes more clear. |
oliverklee
changed the title
oliverklee
force-pushed
the
task/coverage-separate
branch
from
171ea94 to
42501d3
Compare
oliverklee
force-pushed
the
task/coverage-separate
branch
2 times, most recently
from
d561a48 to
5b0705c
Compare
|
Also, we are now not getting a 'Code coverage' on |
| on: | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
Is the indentation wrong here? Something seems to preventing a match.
There was a problem hiding this comment.
Extra spaces should not matter. It looks correct, so IDK why it's not working.
There was a problem hiding this comment.
I've just rebased and repushed, an the coverage job seems to have run fine for me.
There was a problem hiding this comment.
Can't see a problem now, though the checks run this time have been the pull_request checks rather than the push checks. I'll approve and hopefully will see some checks run on 'push' from the merge.
Oh, I've just realized why the code coverage wasn't run - you're pushing to the task/coverage-separate branch, not master - it's only supposed to run on pushes to master. D'oh, sorry.
There was a problem hiding this comment.
The CI should have run for the pull_request trigger, though.
There was a problem hiding this comment.
The CI should have run for the
pull_requesttrigger, though.
Strange. When I reviewed last night, it definitely showed 12 succussful checks on 'push', rather than 13 on 'pull_request'. I wouldn't have spotted this potential problem otherwise.
This makes the CI configuration easier to understand.
oliverklee
force-pushed
the
task/coverage-separate
branch
from
5b0705c to
39390bb
Compare
This makes the CI configuration easier to understand.