laminas / laminas-ci-matrix-action

GitHub Action for creating a CI job matrix.
BSD 3-Clause "New" or "Revised" License
24 stars 15 forks source link

Code Coverage Reporters #4

Open gsteel opened 3 years ago

gsteel commented 3 years ago

Feature Request

Q A
New Feature yes
RFC ?
BC Break ?

Summary

It would be really cool to detect or configure the ability to report coverage after successful PHPUnit actions. I guess these would be vendor specific?

Most laminas repos seem to use coveralls and have an existing coveralls.yml file. I personally tend to favour codecov which can use a config file but doesn't require one.

Is this a feature that you might consider?

gsteel commented 3 years ago

Maybe, adding clover output to GitHub artefacts on the one of the PHPUnit jobs would suffice? Then consumers could add their own coverage processors by fetching the artefact and uploading it to their tool of choice. Would that be possible?

Ocramius commented 3 years ago

Is this a feature that you might consider?

perhaps by looking at declared environment variables, then using those to publish to various external services?

Maybe, adding clover output to GitHub artefacts on the one of the PHPUnit jobs would suffice?

Every tool uses different approaches (clover report, junit report, etc.), so the actual tests to run would depend on the tool to integrate.

weierophinney commented 3 years ago

What we could potentially do for now is to generate the code coverage report when running PHPUnit. You could then use a .laminas-ci/post-run.sh script to upload the report to the location of your choice, using the tool of your choice.

Alternately, one feature I've been considering is one to allow specifying additional checks to include (instead of defining the full set via the .laminas-ci.json "checks" configuration). This would allow running tests with coverage and uploading coverage as a single check that you define directly in your repository.

weierophinney commented 3 years ago

Version 1.3.0 has the ability to specify additional checks to run, besides those auto-discovered now (see #12 for details).

gsteel commented 3 years ago

Thanks @weierophinney – #12 makes it really easy to add another check. I added a composer script say phpunit --coverage-clover and then ran this from additional_checks with the pcov extension stupidly thinking that I'd be able to get hold of coverage.xml inside my next action, in my case codecov/codecov-action@v1 which was queued up by needs: [qa]

The problem is extracting the coverage data file. My initial thoughts were that uses: actions/upload-artifact@v2 in the laminas ci action followed by uses: actions/download-artifact@v2 inside my coverage upload action would be the way to do it, but would that even work for a custom docker runner?

As you previously mentioned, post-run.sh is probably the best thing here. FWIW I got it all stitched together using post-run.sh and for posterity, I'll link to what I did here in case it's useful to anyone else.

weierophinney commented 3 years ago

That's awesome, @gsteel !

So, the laminas-continuous-integration-action and this one both perform a clone operation internally, so that they can do shallow checkouts of just the PR branch and, in the case of this action, the target branch, allowing us to do diffs without too much overhead.

That said... I did some improvements for either the 1.1 or 1.2 series of the laminas-continuous-integration action to allow it to wrk in tandem with actions/checkout; it essentially does not attempt to clone if it detects that the working directory is already populated. This means you could do an actions/checkout step between the CI matrix step (this one) and the continuous-integration step, and then add further steps that operate on the checkout.

I'm going to open an issue on this repo to duplicate the checkout logic from the continuous-integration action to this one.

gsteel commented 3 years ago

I didn't realise that the actions all operate on the same workdir - I assumed everything was torn down for each run. So what you're saying is that workflows could be updated to something like:

That would be really handy!

weierophinney commented 3 years ago

@gsteel Yes! Though the CI matrix has to be one job, and then the CI and any other steps would be the next job, as it consumes the matrix. In a bullet-pointed tree layout:

Generally speaking, I'd likely try and do jobs that are self-contained, rather that rely on later steps to deal with artifacts, as the work will have to differ based on the job that was run in the CI step. Basically, you'd need that next step (and any others you define) to either look for specific artifacts in the checkout, or have some knowledge of what the previous job name was, in order to know whether it needs to trigger or not. This can be done with GHA expressions (e.g., if: contains("coverage", matrix.name)), but it can potentially be brittle, particularly if you want to use the same base workflow everywhere (you either end up with a bunch of unneeded steps everywhere, or you have to carefully compare each iteration to the canonical template when the template changes).

In looking at your job you linked previously, the command could become:

composer test-coverage && bash <(curl -s https://codecov.io/bash)

(This works because we're passing the command to a bash subshell.)

Alternately, your test-coverage script in Composer could become:

"test-coverage": [
    "phpunit --coverage-clover coverage.xml",
    "bash <(curl -s https://codecov.io/bash)"
]

(test that to see if it works; I'm not sure how if the way they handle command processes allows for redirection)

I'd likely try one of those before adding more steps to the workflow, and only change the workflow if you need to add services that QA jobs might depend on (e.g., redis, mysql, etc.).

gsteel commented 3 years ago

Thanks for all of your help @weierophinney, I tried mucking about with running the checkout, matrix job, codecov action just for kicks, and found that nothing in the workdir was available to the final action. That is probably down to my _(mis)_understanding of how actions work…

Also, the main problems with running any of the actual tests and coverage uploads via composer is the lack of environment variables - and whilst I can see how to set env vars for a composer script, I can't see how pass through existing secrets to composer. Ultimately, I simply ran with the bash uploader via post-run.sh

Whilst all env vars are given to docker, meaning that the coverage uploader works as it should, I did notice that PHPUnit is not receiving any env vars during its run in any of the matrix actions. In my case, it means that a bunch of tests are skipped because they communicate with an api url that's expected as an environment variable.

Could an -E be passed to sudo here in the ci action ??

weierophinney commented 3 years ago

Could an -E be passed to sudo here in the ci action ??

Absolutely! Please send a PR for that!