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

Use a separate job for each test suite in GitHub CI #4276

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

neilmayhew
Copy link
Contributor

@neilmayhew neilmayhew commented Apr 18, 2024

Closes #4039

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@neilmayhew neilmayhew force-pushed the neilmayhew/4039-ci-separate-test-jobs branch 5 times, most recently from 395bf93 to 987737b Compare April 24, 2024 21:15
@neilmayhew neilmayhew marked this pull request as ready for review April 24, 2024 21:16
@neilmayhew neilmayhew requested a review from lehins April 24, 2024 21:16
@neilmayhew
Copy link
Contributor Author

This still does some unnecessary rebuilding, but it's relatively minor so I think we should merge it as it is now, and follow up with an improvement to stop the rebuilding.

@lehins
Copy link
Contributor

lehins commented Apr 25, 2024

I am not 100% sure about this, but from the looks of it occasionally test suites are not being rebuild, however during restoration of artifacts permissions are probably not restored correctly, which leads to prebuild executable not having an executable permission for the use and leading to Permission denied error. This is just my hypothesis.

Running 1 test suites...
Test suite tests: RUNNING...
/home/runner/work/cardano-ledger/cardano-ledger/dist-newstyle/build/x86_64-linux/ghc-9.8.2/vector-map-1.1.0.0/t/tests/build/tests/tests: createProcess: posix_spawnp: permission denied (Permission denied)
Error: cabal: Tests failed for test:tests from vector-map-1.1.0.0.

Besides that I love the Haskell script in this PR and overall approach looks great.

@neilmayhew
Copy link
Contributor Author

I am not 100% sure about this, but from the looks of it occasionally test suites are not being rebuild, however during restoration of artifacts permissions are probably not restored correctly, which leads to prebuild executable not having an executable permission for the use and leading to Permission denied error. This is just my hypothesis.

If this was the case, I wouldn't expect those tests to have succeeded for ghc 8.10.7 and 9.2.8. Also, I've had everything green on a previous test run that used the artifacts approach. I'll retry those tests and see if they pass. That doesn't mean there isn't a problem, of course, but it will shed more light on the problem.

Besides that I love the Haskell script in this PR and overall approach looks great.

Thanks!

@lehins
Copy link
Contributor

lehins commented Apr 25, 2024

If this was the case, I wouldn't expect those tests to have succeeded for ghc 8.10.7 and 9.2.8.

Not necessarily. See the build for those ghc versions and you will see that the test suite is being recompiled, eg for ghc-9.2.8:

Building test suite 'non-integral-test' for non-integral-1.0.0.0..
[1 of 2] Compiling Tests.Cardano.Ledger.NonIntegral ( test/Tests/Cardano/Ledger/NonIntegral.hs, /home/runner/work/cardano-ledger/cardano-ledger/dist-newstyle/build/x86_64-linux/ghc-9.2.8/non-integral-1.0.0.0/t/non-integral-test/build/non-integral-test/non-integral-test-tmp/Tests/Cardano/Ledger/NonIntegral.o )
[2 of 2] Compiling Main             ( test/Tests.hs, /home/runner/work/cardano-ledger/cardano-ledger/dist-newstyle/build/x86_64-linux/ghc-9.2.8/non-integral-1.0.0.0/t/non-integral-test/build/non-integral-test/non-integral-test-tmp/Main.o )
Linking /home/runner/work/cardano-ledger/cardano-ledger/dist-newstyle/build/x86_64-linux/ghc-9.2.8/non-integral-1.0.0.0/t/non-integral-test/build/non-integral-test/non-integral-test ...
Running 1 test suites...
Test suite non-integral-test: RUNNING...

@neilmayhew
Copy link
Contributor Author

This is working better now that I'm using a tar archive. However, I discovered that the default format (gnu) has one-second resolution on modification times, and I think that's why there are still some rebuilds. I'm changing it to use pax format which has finer resolution.

@neilmayhew neilmayhew force-pushed the neilmayhew/4039-ci-separate-test-jobs branch 4 times, most recently from 24d80fe to eeee2de Compare April 26, 2024 00:31
@lehins lehins force-pushed the neilmayhew/4039-ci-separate-test-jobs branch from eeee2de to b9e3229 Compare April 26, 2024 03:50
@lehins lehins enabled auto-merge April 26, 2024 03:50
@lehins lehins disabled auto-merge April 26, 2024 04:19
@lehins lehins enabled auto-merge April 26, 2024 04:21
Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This was a long awaited improvement to our CI.
Thank you for getting it done!

@lehins lehins merged commit 27baa60 into master Apr 26, 2024
125 checks passed
@lehins lehins deleted the neilmayhew/4039-ci-separate-test-jobs branch April 26, 2024 05:20
@neilmayhew
Copy link
Contributor Author

A discussed in meetings, the tests now wait for all of the builds to finish instead of starting when the one with the same ghc version finishes. If we ever want to improve on this, there's a solution here. (Max pointed me to this discussion.)

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.

Create separate GithubActions jobs for each test suite
2 participants