ci: ensure tests use build output and simplify CI/CD workflow #9649
Conversation
Thanks, @takaokouji, for making me take a closer look at the caching behavior in this workflow. Subtle and dangerous! In the previous version, the `build/` directory was cached with this key: ```yaml key: ${{ runner.os }}-build-${{ hashFiles('package-lock.json') }} ``` As @takaokouji observed, this means that if a commit or PR doesn't change `package-lock.json`, the cache key will not change. Two unrelated builds, possibly with very different content in `src/` or elsewhere, could share the same build cache. In this case, when the later testing steps restore the cache, they'll both be testing the same build output. One of them won't be testing what you expect! Luckily, the deploy steps also used the same cache keys, so they also didn't deploy what you expect. I say "luckily" because the alternative is to release something that didn't run any tests. All the caching and retrieval steps also took a lot of time, enabled only a tiny amount of extra parallelism (in the deploy steps), and added a lot of complexity to the workflow. Removing all of that means a faster, easier-to-understand workflow with no sneaky gremlins making us test or deploy the wrong thing.
|
For my first test, this new workflow took about 10 minutes to run. The previous workflow was more like 20 minutes. |
|
OK, sorry for the churn. This is genuinely ready now, I think 😅 Commit dea503f was because I realized that the build & test workflow didn't run on this PR, and we do want to test PRs! Commit 7bb299b was because, once the build & test workflow did run on this PR, I realized that it was attempting to run the deploy steps. That's good for |
.github/workflows/ci-cd.yml
Outdated
| - run: | | ||
| for F in chrome chromium chromedriver; do | ||
| which $F && $F --version || echo Not found: $F | ||
| done | ||
| - name: Run Integration Tests | ||
| env: | ||
| JEST_JUNIT_OUTPUT_NAME: results.txt | ||
| JEST_JUNIT_OUTPUT_DIR: test-results/integration | ||
| run: npm run test:integration -- --reporters="default" --reporters="jest-junit" | ||
| - name: Store Integration Test Results | ||
| uses: actions/upload-artifact@a8a3f3ad30e3422c9c7b888a15615d19a852ae32 # v3 | ||
| with: | ||
| name: integration-test-output | ||
| path: ./test-results/* |
There was a problem hiding this comment.
With the the env variables as they are the integration tests artifact would contain both unit and integration tests result.
There was a problem hiding this comment.
Ah, good point - thanks!
There was a problem hiding this comment.
I combined the two steps for storing test results, so now there's one artifact that contains any/all output from both testing steps. I set it to run "always" so that (for example) if the unit tests have output but the build fails, the unit test output is still available. That also means any output from failed tests will be available, which probably should have been the way we set it up to begin with!
|
I think it is safe to cache |
|
The |
|
🎉 This PR is included in version 4.0.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks, @takaokouji, for making me take a closer look at the caching behavior in this workflow. Subtle and dangerous!
Resolves
Proposed Changes
In the previous version, the
build/directory was cached with this key:Later workflow steps would restore the build output with this same key.
The change proposed in this PR combines the steps together, eliminating the need to save and restore caches between steps.
Reason for Changes
As @takaokouji observed in #9643, this means that if a commit or PR doesn't change
package-lock.json, the cache key will not change. Two unrelated builds, possibly with very different content insrc/or elsewhere, could share the same build cache. In this case, when the later testing steps of the two builds each restore the cache, they'll both be testing the same build output. One of them won't be testing what you expect!Luckily, the deploy steps also used the same cache keys, so they also didn't deploy what you expect. I say "luckily" because the alternative is to release something that didn't run any tests.
All the caching and retrieval steps also took a lot of time, enabled only a tiny amount of extra parallelism (in the deploy steps), and added a lot of complexity to the workflow. Removing all of that means a faster, easier-to-understand workflow with no sneaky gremlins making us test or deploy the wrong thing.