test(install-dynamic-plugins): cover the unrecognised pullPolicy path, document two unreachable guards (RHIDP-16761) - #4555
Open
gustavolira wants to merge 2 commits into
Conversation
…able (RHIDP-16761) installer-oci.ts had three branches that no test could ever reach, and they read as a coverage gap to anyone looking at the report: :45 splitOciPackage if (!imagePart || !pluginPath) return null :140 isAlreadyInstalled if (pullPolicy !== PullPolicy.ALWAYS) return false :147 isAlreadyInstalled if (!parts) return false They are unreachable, verified rather than assumed. installOciPlugin is not part of the package's public API — index.ts exports only the CLI module — so its sole caller is installer.ts, downstream of merger.ts calling ociPluginKey, whose OCI_REGEX already rejects a leading or trailing `!`. Those forms are in oci-key.test.ts's invalidCases, which is why :45 and :147 cannot fire. :140 cannot fire because PullPolicy has exactly two members and IF_NOT_PRESENT already returned three lines above. Three reviewers on RHIDP-16222 proposed deleting them. Keeping them is the better trade: deleting :147 in particular downgrades a clean skip into a TypeError on parts.imagePart the moment the upstream validation is relaxed, and the guards cost nothing at runtime. So they stay, and each carries an istanbul ignore naming the invariant that makes it unreachable — which is the part that matters. If someone loosens OCI_REGEX or adds a third PullPolicy, the comment tells them the pragma is now wrong. The `/* istanbul ignore next -- reason */` form follows the one existing use in this repo, augment-backend's HttpEmbedder.ts. Measured, full suite with --no-cache: before 95.08% stmts / 90% branch, uncovered lines 45, 140, 147 after 100% stmts / 100% branch / 100% funcs / 100% lines No behaviour change: the guards still run, and the same 258 tests pass. tsc, prettier and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4555 +/- ##
==========================================
- Coverage 62.31% 62.30% -0.01%
==========================================
Files 2606 2606
Lines 104644 104642 -2
Branches 29391 29389 -2
==========================================
- Hits 65204 65202 -2
Misses 37623 37623
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:
|
… (RHIDP-16761)
Review of the previous commit. One of the three guards it called unreachable is
reachable from a user's config, so the claim was wrong and the istanbul ignore
was hiding a real branch rather than documenting a dead one.
`isAlreadyInstalled`'s `if (pullPolicy !== PullPolicy.ALWAYS) return false` was
justified on the grounds that PullPolicy has exactly two members and
IF_NOT_PRESENT already returned above. That reasoning only holds if the value is
actually one of the two. It is not checked: installer.ts reads the config with
`parseYaml(rawContent) as DynamicPluginsConfig`, a type assertion with no
runtime validation, and `effectivePullPolicy` returns `plugin.pullPolicy`
whenever it is truthy. So `pullPolicy: Never` — or `always`, lower-cased —
travels from dynamic-plugins.yaml straight into this comparison.
The behaviour on that path is the right one: fall through and re-install, rather
than silently skip and strand the plugin at whatever version is on disk. It just
had no test. It does now, and removing the guard fails exactly that test and
nothing else.
The pragma on that line is gone, replaced by a comment saying why the branch
exists. The line is now covered for real instead of excluded from the report.
The other two remain ignored, with better reasons than the first commit gave.
The original comments cited OCI_REGEX, which only explains why a trailing `!` is
rejected. The reason a package with no `!` at all never reaches splitOciPackage
with an empty side is that the merger appends the resolved path first —
`!plugin.package.includes('!')` in mergePlugin, and resolveInherit for the
{{inherit}} case. Both mechanisms are now named, so a reader can check whether
the pragma still holds after changing either.
installer-oci.ts stays at 100% on all four metrics, but one of those branches is
now covered by a test rather than excluded by a pragma.
259 tests / 19 suites. tsc, prettier and lint clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gustavolira
changed the title
|
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
Covers one branch in
installer-oci.tsthat a review of this PR's own first commit proved is reachable, and documents two that are not.Closes RHIDP-16761. Follow-up to #4526 (RHIDP-16222).
The correction that shaped this PR
The first commit claimed three guards were unreachable and put an
istanbul ignoreon each. One of those claims was wrong, and the pragma was hiding a real branch rather than documenting a dead one.The justification was that
PullPolicyhas exactly two members andIF_NOT_PRESENTalready returned above. That only holds if the value really is one of the two, and nothing checks.installer.tsreads the config withparseYaml(rawContent) as DynamicPluginsConfig— a type assertion, no runtime validation — andeffectivePullPolicyreturnsplugin.pullPolicywhenever it is truthy. SopullPolicy: Never, or a lower-casedalways, travels from a user'sdynamic-plugins.yamlstraight into this comparison.The behaviour on that path is the right one: fall through and re-install, rather than silently skip and strand the plugin at whatever version happens to be on disk. It simply had no test. It has one now, and removing the guard fails exactly that test and nothing else.
What is ignored, and why
The two remaining pragmas stand, with better reasons than the first commit gave. The original comments cited
OCI_REGEX, which only explains why a trailing!is rejected. The reason a package with no!at all never reachessplitOciPackagewith an empty side is that the merger appends the resolved path first —!plugin.package.includes('!')inmergePlugin, andresolveInheritfor the{{inherit}}case. Both mechanisms are named in the comments now, so a reader can check whether the pragma still holds after changing either.They stay rather than being deleted: dropping the second downgrades a clean skip into a
TypeErroronparts.imagePartthe moment upstream validation is relaxed, and both cost nothing at runtime.The
/* istanbul ignore next -- reason */form follows the one existing use in this repo,augment-backend'sHttpEmbedder.ts:47.Coverage
installer-oci.tswas 95.08% statements / 90% branches with lines 45, 140 and 147 uncovered. It is 100% on all four metrics now — but the difference that matters is that thepullPolicybranch got there by being tested, not by being excluded.Mutation
if (pullPolicy !== PullPolicy.ALWAYS) return falseremovedre-installs rather than skipping when the configured pullPolicy is not a recognised valueChecks
yarn tsc— cleanyarn prettier:check— cleanyarn lint:all— cleanCI=true yarn test— 19 suites, 259 tests, all passingNo behaviour change, no changeset.
Follow-up worth considering, not done here
pullPolicyis not validated at parse time. Falling through to a re-install is a safe default, but an operator who typos it gets no warning and a plugin that re-downloads on every run. Validating the value and logging when it is unrecognised would be a small, separate change — out of scope for a PR about coverage pragmas.🤖 Generated with Claude Code