fix(ci): update the setup-workspace cache assertions to the widened path list - #9603
Merged
Conversation
…ath list #9596/#9597 changed the node_modules cache to glob every workspace and added a generation salt to the key, but the composite-action test still asserted the old literal `apps/loopover-ui/node_modules`, so it has been failing on main and on every branch since. Assert the contract that now holds, and tighten it in two places the old test left open: - both workspace globs, matching what package.json declares; - the `npm-v2-` salt, asserted beside the paths, because widening the paths without bumping the salt is a no-op and the two must move together; - the SAVE step's path list equals the RESTORE step's -- the save list decides what a future run can restore, so a narrowing there would stay invisible until some later job failed on a missing workspace.
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9603 +/- ##
===========================================
+ Coverage 75.56% 89.04% +13.48%
===========================================
Files 276 880 +604
Lines 58198 111175 +52977
Branches 6241 26473 +20232
===========================================
+ Hits 43975 98994 +55019
+ Misses 13953 11155 -2798
- Partials 270 1026 +756
Flags with carried forward coverage won't be shown. Click here to find out more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The break
test/unit/ci-composite-setup-workspace.test.tshas been failing onmainand on every open branch since #9596/#9597 merged:My fault: those PRs changed the cache to glob every workspace and added a key salt, but I didn't update the test that pins the old literal path.
The fix
Assert the contract that now holds, and close two gaps the old test left open while I'm here:
apps/*/node_modules,packages/*/node_modules), matching whatpackage.jsondeclares as workspaces.npm-v2-salt, asserted beside the paths. Widening the paths without bumping the salt is a no-op — that's exactly the bug fix(ci): bump the cache-key salt so #9596's widened path list actually takes effect #9597 had to fix after fix(ci): cache every workspace's node_modules, not just the root and one app #9596 — so the test now fails if a future change moves one without the other.Verified locally: 5/5 passing.