test(install-dynamic-plugins): cover getTarball's manifest guards and download dedup (RHIDP-16760) - #4554
Open
gustavolira wants to merge 2 commits into
Open
Conversation
… dedup (RHIDP-16760)
The deep review of RHIDP-16222 left image-cache.ts covered for getPluginPaths and
getDigest but not for getTarball: lines 109-123, the whole of
downloadAndLocateTarball, were uncovered, and nothing asserted the promise cache
the class docblock is written around.
The two InstallException guards matter for the same reason RHDHBUGS-2439 did.
They are readable-failure paths an operator reads out of the init container log,
and dropping either turns the failure into path.join(localDir, undefined) rather
than a message naming the image.
The cache is the class's stated reason to exist — several plugins in one overlay
image share a single skopeo copy — and it had no test at all. Nor did the
failure eviction, which is what stops one transient registry error from being
replayed to every later caller for the life of the process.
Faked at the seam the siblings already use: a fake skopeo shell binary that
materialises manifest.json plus the layer blob at the dir: destination
(extra-catalog-index.test.ts), with the invocation log that lets the dedup tests
count forks (skopeo.test.ts). No network, no registry, no jest.mock.
Both directions of the cache are covered, because a dedup test on its own would
pass against a cache that collapsed every image onto one download: three
concurrent calls for one image fork skopeo once, two calls for different images
fork twice.
Mutation-checked, each mutant reverted after its run:
no-layers guard returns a path instead of throwing
-> 2 failures, both no-layers tests
malformed-digest guard returns a path instead of throwing
-> 1 failure
layer path returned unjoined
-> 2 failures
promise cache removed from getTarball
-> 1 failure, the concurrent-callers test
pending.catch eviction removed
-> 1 failure, the retry test
19 suites / 265 tests. tsc, prettier and lint clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4554 +/- ##
=======================================
Coverage 62.31% 62.31%
=======================================
Files 2606 2606
Lines 104644 104644
Branches 29391 29391
=======================================
+ Hits 65204 65213 +9
+ Misses 37623 37614 -9
Partials 1817 1817
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…table (RHIDP-16760) Review of the previous commit. It shipped two separate tests for the same error — a manifest with `"layers": []` and one with no `layers` key — and the mutation run should have been the tell: both died together, neither independently. Measured with --no-cache: dropping the second changes no covered statement and no covered branch path. Both shapes reach the same guard through the same `manifest.layers?.[0]?.digest` optional chain, so as two `it` blocks the second was documentation dressed as coverage. That is the exact criticism levelled at the W10= case during the review of redhat-developer#4526, and it applies here too. Folded into one `it.each` table, which is what they are: equivalent inputs to one assertion. Same form oci-key.test.ts uses for its invalidCases list. The comment records that the equivalence was measured rather than assumed, so nobody re-splits them later on the theory that they cover different branches. No coverage change, by construction. 19 suites / 265 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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.
What
Adds seven tests for
OciImageCache.getTarballand its private helperdownloadAndLocateTarball, which had no coverage at all. Test-only; no production file is touched.Closes RHIDP-16760. Follow-up to #4526 (RHIDP-16222), where this gap was found and deliberately left out.
Why this was not in #4526
#4526 added
image-cache.test.tsforgetPluginPathsandgetDigestonly. Three independent review passes on that PR flagged the same remaining hole, and it was left out on purpose: it is pre-existing code that PR neither touched nor broke, and folding it in would have turned a regression-test PR into a coverage sweep.Measured on
mainbefore this PR,image-cache.tssits at 85.24% statements / 66.66% branches with lines 109-123 uncovered — the whole ofdownloadAndLocateTarball.What is covered
The two readable-failure guards. These matter for the same reason RHDHBUGS-2439 did: they are what an operator reads out of the init container log. Drop either one and the failure becomes
path.join(localDir, undefined)instead of a message naming the image."layers": []and one with nolayerskey at all, as oneit.eachtable — measured, the two shapes cover no distinct branch, so they are equivalent inputs to one assertion rather than two testsnocolon), asserting both theInstallExceptiontype and the message naming the offending digestThe promise cache, which is the class docblock's stated reason to exist — several plugins in one overlay image sharing a single
skopeo copy— and which had no test.getTarballcalls for one image forkskopeoonce, and all three resolve to the same pathBoth directions, because a dedup test on its own would pass against a cache that collapsed every image onto one download.
The failure eviction (
pending.catch(() => this.tarballs.delete(resolved))), which is what stops one transient registry error from being replayed to every later caller for the life of the process. The fake fails its first invocation and succeeds afterwards; the test asserts the second call retries and succeeds.The success path — the returned path is the layer blob inside the cache directory, and the
skopeoargv is asserted verbatim.Seam
No network, no registry, no
jest.mock— there is not one in this directory. The fakeskopeois a shell binary that materialisesmanifest.jsonplus the layer blob at thedir:destination, exactly asextra-catalog-index.test.tsdoes, with the invocation log fromskopeo.test.tsso the dedup tests can count forks.Mutation results
Each test was made to fail before being kept: break the behaviour it covers, run the full suite, revert.
names the offending digest ...return localDir)getTarballshares one skopeo copy between concurrent callerspending.catcheviction removedevicts a failed download ...No test survived its own mutation.
Checks
yarn tsc— cleanyarn prettier:check— cleanyarn lint:all— cleanCI=true yarn test— 19 suites, 265 tests, all passingNo changeset: test-only, no published behaviour change.
Still not covered
image-cache.tsline 94,if (entry && typeof entry === 'object')ingetPluginPaths— the falsy-entry path, which a malformed annotation like[null, "x", {"plugin":{}}]would exercise. It leaves the file at 94.44% branches. Left alone deliberately: it is pre-existing code in a different method from this PR's subject, and the same blast-radius rule that keptgetTarballout of #4526 keeps this out of here.run.ts'schild.on('error')path — the variant where the binary exists but cannot be executed. Separate gap, not filed;Skopeo's constructor already throws for askopeothat is missing fromPATH, so the common field case is handled elsewhere.🤖 Generated with Claude Code