-
Notifications
You must be signed in to change notification settings - Fork 63
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
Measure coverage for integration tests in CI #1893
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments regarding documentation and logging.
One concern I do have is adding hpc
as part of the matrix. This results in needing a lot of if: matrix.hpc == boolval
spread around, and also has the risk that future modifications to the matrix will cause hpc to run too much or too little (IIUC, hpc should only be run once, for integration-tests, under ubuntu, and nowhere else). YAML files don't really allow encapsulation/re-use unless the interpreter is setup that way, so we'd have to duplicate things, but I'm wondering if splitting out the hpc testing into a separate job (and removing that from the cabal-test
would be overall cleaner/simpler. Another thing I just thought of as well: can you think of any downsides to not having a non-hpc ubuntu build + artifacts?
@kquick The only downside I can think of is that HPC builds generate |
@@ -182,28 +193,44 @@ jobs: | |||
SIGNING_KEY: ${{ secrets.SIGNING_KEY }} | |||
run: .github/ci.sh sign $NAME-with-solvers.tar.gz | |||
|
|||
- if: matrix.ghc == '8.10.7' | |||
- if: matrix.ghc == '8.10.7' && matrix.hpc == false | |||
uses: actions/upload-artifact@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we explicitly checking matrix.hpc == false
here? If the ghc version is because this is (arbitrarily) our current "distribution" version, won't this break if we then happen to choose the distribution version to be the same one we are generating coverage for? Same question goes for the next 2 steps with the same check. Either something to fix, or something that should have an explanatory comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is here to avoid the problem you identified. If the distribution version is changed to match the HPC version, then this prevents the HPC binaries from clobbering the distribution binaries. For example, assume the distribution is changed to 9.4.4
. The matrix specifies that there are two different Ubuntu builds with GHC 9.4.4. One of those builds has HPC enabled and the other has HPC disabled. This conditional ensures that it's the version with HPC disabled that gets uploaded. I'll add a comment to clarify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment clarifying the matrix.hpc == false
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you still think there's a logic error here
.github/workflows/ci.yml
Outdated
uses: actions/upload-artifact@v2 | ||
with: | ||
path: dist/bin | ||
name: ${{ runner.os }}-bins | ||
|
||
- if: matrix.run-tests && matrix.hpc == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here (and in the next two steps), why are we concerned with matrix.run_tests
?
In fact, for this entire job, it's not clear to me what matrix.run_tests
is intended to solve, and why we need it at all in this job. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like matrix.run_tests
exists to avoid uploading build artifacts from the ubuntu-20.04
build because none of the testing runs use that version of Ubuntu. Does that sound right to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, run_tests
is implicitly true for the HPC builds so I removed it from the HPC related steps.
There are some external customers, and a sudden proliferation of |
A couple comments:
@kquick I think this is ready to merge, unless there's anything else you'd like to see addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This change adds additional steps to the CI system that:
A few things to note:
index.html
file there that links to all available reports