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

Measure coverage for integration tests in CI #1893

Merged
merged 49 commits into from
Sep 15, 2023
Merged

Measure coverage for integration tests in CI #1893

merged 49 commits into from
Sep 15, 2023

Conversation

bboston7
Copy link
Contributor

@bboston7 bboston7 commented Jul 12, 2023

This change adds additional steps to the CI system that:

  1. Builds a version of SAW with HPC support enabled
  2. Runs the integration tests with the binaries produced in (1)
  3. Generates an HTML coverage report from the HPC data from (2) and uploads this as an artifact tagged with the appropriate PR number
  4. Downloads all coverage artifacts from all PRs and generates a website containing all of them (much like the cryptol docs).
  5. Uploads this website to github pages

A few things to note:

  • The coverage reports are hosted at https://galoisinc.github.io/saw-script
    • Step (4) generates the index.html file there that links to all available reports
  • Coverage reports are available so long as the underlying artifact is available. This is determined by a combination of the expiration date of the artifact (90 days after it is generated), as well as our artifact storage limits. This should be sufficient for coverage reports of active PRs, as well as history going back a bit.

@bboston7 bboston7 self-assigned this Jul 12, 2023
@bboston7 bboston7 marked this pull request as ready for review August 28, 2023 21:45
@bboston7 bboston7 requested a review from kquick August 28, 2023 21:45
Copy link
Member

@kquick kquick left a 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?

.github/ci.sh Outdated Show resolved Hide resolved
.github/ci.sh Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/ci.sh Outdated Show resolved Hide resolved
.github/ci.sh Show resolved Hide resolved
compute-coverage.sh Outdated Show resolved Hide resolved
@bboston7
Copy link
Contributor Author

bboston7 commented Sep 8, 2023

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 .tix files when running SAW. I think this is fine in CI, but might be surprising to anyone who downloads these binaries to run locally. That being said, do we think people who aren't SAW developers are downloading and running CI artifacts? If not, what do you think about me removing the hpc flag from the build matrix and always building the ubuntu-22.04 binary with HPC enabled?

.github/workflows/ci.yml Show resolved Hide resolved
@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

uses: actions/upload-artifact@v2
with:
path: dist/bin
name: ${{ runner.os }}-bins

- if: matrix.run-tests && matrix.hpc == true
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@kquick
Copy link
Member

kquick commented Sep 8, 2023

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 .tix files when running SAW. I think this is fine in CI, but might be surprising to anyone who downloads these binaries to run locally. That being said, do we think people who aren't SAW developers are downloading and running CI artifacts? If not, what do you think about me removing the hpc flag from the build matrix and always building the ubuntu-22.04 binary with HPC enabled?

There are some external customers, and a sudden proliferation of .tix files might be annoying, confusing, or even problematic. It's not clear to me though whether external users would get this version (unless we changed our target distribution version from 8.10.7), so perhaps that's not a legitimate concern. I would not expect internal devs to be concerned, even if they did pull down the hpc artifacts.

@bboston7
Copy link
Contributor Author

A couple comments:

  • I resolved merge conflicts with the new solver caching work and enabled caching on the coverage tests.
  • Given that the binaries we distribute come from the build artifacts I think it's best to keep the HPC and non-HPC builds separate, because we don't want HPC enabled on our distribution binaries.

@kquick I think this is ready to merge, unless there's anything else you'd like to see addressed.

Copy link
Member

@kquick kquick left a comment

Choose a reason for hiding this comment

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

Looks good!

@bboston7 bboston7 merged commit 1f31781 into master Sep 15, 2023
38 checks passed
@bboston7 bboston7 deleted the bb/ci-coverage branch September 15, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants