Test CI overhaul #2209
Test CI overhaul #2209
Conversation
tbro
requested review from
philippecamacho,
ImJeremyHe,
sveitser,
jbearer and
imabdulbasit
as code owners
tbro
force-pushed
the
tb/test-tests
branch
4 times, most recently
from
65c64c4 to
eb7dc07
Compare
tbro
changed the title
.github/workflows/test.yml
Outdated
|
|
||
| - uses: actions/checkout@v4 | ||
| with: | ||
| # By default actions/checkout checks out a merge commit. Check out the PR head instead. |
There was a problem hiding this comment.
Why do we want to check out the PR head instead?
There was a problem hiding this comment.
I might be missing something but I thought we want to merge the PR into main and use that to run the CI so that we are confident things work after a future merge into main.
There was a problem hiding this comment.
Yea I guess I'm confused about what is happening there.
.github/workflows/test.yml
Outdated
| - name: Enable Rust Caching | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| cache-on-failure: true |
There was a problem hiding this comment.
Is this a good idea? Doesn't it mean that if we have a failure we only get a partial cache and if we afterwards get a successful build for the same cache key we don't save it and keep a partial cache?
There was a problem hiding this comment.
I don't understand why it would prevent the cache from being saved on a successful run. But I'll remove it isn't adding much any way.
| @@ -41,25 +43,23 @@ jobs: | |||
| with: | |||
| submodules: recursive | |||
|
|
|||
| - name: Configure Environment | |||
| run: PATH="$PWD/target/debug:$PATH" | |||
There was a problem hiding this comment.
I'm surprised this works. I thought one has to write it to $GITHUB_ENV to persist between step
There was a problem hiding this comment.
We made this change in test.yaml a while back. I think it works b/c we are running all the steps on the same host... so we don't need any additional persistence. Also we are not creating a new variable, we are simply appending to PATH and PATH is always exported on the host. For it not to work I think github would have to do some variable resetting between steps.
| @@ -7,7 +7,7 @@ if [ -z "${IN_NIX_SHELL-}" ]; then | |||
| REPO_ROOT="$(dirname "$(dirname "$(readlink -fm "$0")")")" | |||
| # Default to CARGO_TARGET_DIR if set, otherwise use the default target directory. | |||
| TARGET_DIR="${CARGO_TARGET_DIR:-${REPO_ROOT}/target}" | |||
| export "PATH=${TARGET_DIR}/release:$PATH" | |||
| export "PATH=${TARGET_DIR}/debug:$PATH" | |||
There was a problem hiding this comment.
Hmm I think this will confuse people who have been building with release. I think we should log something prominent to the terminal about this change.
Speeds up tests in CI. Total duration is now < 20 min. Currently on
main tests take about twice as long. You can see the successful
workflow here.
* instead of --release we set a lower optimization level
* packages that require higher optimization are specified individually
* application and test binaries are built in parallel
* unit tests and integration/e2e tests are run in parallel
Switching from --release to test profile has the benefit of shorter
build times as well as catching more errors. There is a fix included
for builder using duplicated -m short options. This sort of error will
cause some tests to fail now with these changes. And availability
options were added to more process-compose services to catch more
failures early. An additional improvement would be to run slow tests
in parallel w/ unit tests. I'm not currently aware of a way to include
slow tests in this workflow w/out making it required for merge.
This PR:
Speeds up tests in CI. Total duration is now < 20 min. Currently on
maintests take about twice as long. You can see the successful workflow here.--releasewe set a lower optimization levelSwitching from
--releasetotestprofile has the benefit of shorter build times as well as catching more errors. There is a fix included for builder using duplicated-mshort options. This sort of error will cause some tests to fail now with these changes. And availability options were added to more process-compose services to catch more failures early. An additional improvement would be to run slow tests in parallel w/ unit tests. I'm not currently aware of a way to include slow tests in this workflow w/out making it required for merge.How to test this PR:
If tests pass faster than what we see in earlier commits, its a winner.