Coverage uplift across config, fetcher, updater and multirepo#737
Open
rdimitrov wants to merge 9 commits into
Open
Coverage uplift across config, fetcher, updater and multirepo#737rdimitrov wants to merge 9 commits into
rdimitrov wants to merge 9 commits into
Conversation
rdimitrov
changed the title
rdimitrov
force-pushed
the
config-coverage-followup
branch
from
June 3, 2026 14:58
bbb7f54 to
123dcd5
Compare
There was a problem hiding this comment.
Pull request overview
This PR increases statement coverage across several core metadata packages by adding/expanding unit tests, making fetcher tests hermetic (no external network dependency), and removing dead test helper documentation/code in internal/testutils.
Changes:
- Add table-driven tests to cover previously-uncovered config/fetcher/updater/multirepo branches.
- Make
metadata/fetcherdownload tests hermetic viahttptest.Server, including an additional body-size limit case. - Remove unused updater-specific test helpers and update
internal/testutilsdocumentation accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| metadata/updater/updater_test.go | Adds new tables for GetTargetInfo, DownloadTarget, and FindCachedTarget, plus a Windows skip helper for simulator routing limitations. |
| metadata/multirepo/multirepo_test.go | Adds table tests for EnsurePathsExist and failure-path tests for New(). |
| metadata/fetcher/fetcher_test.go | Reworks DownloadFile tests to use httptest.Server and adds tests for constructors/setters and size-limit behavior. |
| metadata/config/config_test.go | Adds tests covering UpdaterConfig fetcher configuration helpers and retry options forwarding. |
| internal/testutils/README.md | Removes references to deleted updater helper utilities. |
| internal/testutils/helpers/updater.go | Deletes unused updater-related helper/test harness code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The four mutator methods on UpdaterConfig that proxy to the underlying DefaultFetcher (SetDefaultFetcherHTTPClient, SetDefaultFetcherTransport, SetDefaultFetcherRetry, SetRetryOptions) were sitting at 0% statement coverage. They share the same shape -- type-assert cfg.Fetcher to a *fetcher.DefaultFetcher, return an error if it's not, otherwise proxy the call -- so a small nonDefaultFetcher test double plus a newCfgWithFetcher helper unlocks both branches for all four. Adds four table-driven tests, one per method: * TestSetDefaultFetcherHTTPClientTable -- 2 cases. * TestSetDefaultFetcherTransportTable -- 2 cases. * TestSetDefaultFetcherRetryTable -- 3 cases (zero counts included). * TestSetRetryOptionsTable -- 3 cases (nil opts, max-tries opt, wrong fetcher type). go test -cover ./metadata/config/ climbs from 28.6% to 97.1% of statements. The remaining 2.9% is the inner df.SetTransport error path, which only fires when the fetcher's internal httpClient isn't a *http.Client -- the field is unexported so reaching it from outside the fetcher package would require reflection. Left as-is. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
internal/testutils/helpers/updater.go defined UpdaterTestCase, TrustedMetadataTestCase, CheckError, AssertFilesExist, AssertFilesExact and RunTrustedMetadataTests, all of which have zero callers anywhere in the repository. The actual updater and trustedmetadata test files use simulator.TestRepository's built-in AssertFilesExist/Exact and direct assert.ErrorIs/ErrorContains, not these helpers. The README's "Updater utilities" section additionally listed ContainsTargetWithPrefix, AssertTargetDownloaded and AssertTargetNotDownloaded -- none of which are defined anywhere in the helpers package either. Remove that stale section too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
NewFetcherWithHTTPClient, NewFetcherWithRoundTripper, SetHTTPClient, SetTransport, SetRetry and SetRetryOptions were all at 0% statement coverage. Add focused tests for each that exercise the observable state changes (the test file is in the same package, so we can verify the unexported client/retryOptions fields directly). SetTransport gets a two-case table covering both branches: success on a fetcher backed by *http.Client, and the type-assertion error when the underlying client is a mock httpClient. go test -cover ./metadata/fetcher/ climbs from 57.1% to 87.8%. The remaining 12.2% sits inside DownloadFile and is the network-dependent error paths (length mismatch via Content-Length, retry on HTTP errors); those are best addressed in a separate change that swaps the live jku.github.io URLs for a httptest.Server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
The TestDownLoadFile + TestDownloadFile_NoHTTPClientSet table previously hit jku.github.io/tuf-demo for the success and length-mismatch cases, which made CI dependent on an external service and prevented us from covering error-injection paths in DownloadFile that aren't reachable against a live server. Replace the live URLs with a tiny httptest.Server fixture (newRootServer) that serves the sampleRootData fixture on /1.root.json with a correct Content-Length, and 404s on every other path. Add a TestDownLoadFile_BodyExceedsLimit case to cover the post-read LimitReader path inside DownloadFile (server omits Content-Length but returns 200 bytes when maxLength is 10). Net coverage: fetcher 87.8% -> 93.9% of statements (DownloadFile specifically 81.2% -> 90.6%). All tests are now offline-safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
GetTargetInfo (40%), DownloadTarget (0%) and FindCachedTarget (0%) weren't exercised by the table-driven updater suite. Add three focused tables backed by simulator.TestRepository: * TestGetTargetInfoTable -- known vs unknown target lookup. * TestDownloadTargetTable -- happy path with cfg.RemoteTargetsURL, happy path with an explicit baseURL argument, and the no-base-URL-set rejection. * TestFindCachedTargetTable -- post-download cache hit, missing file, corrupted (hash-mismatch) file, and the DisableLocalCache no-op. The simulator's URL routing for targets has two latent bugs: its lastIndex helper panics on target paths with fewer than two separators, and the consistent-snapshot hash-prefix parser mis-handles nested paths. We sidestep both by using "a/b/<name>" paths and disabling consistent_snapshot in the download tests -- worth fixing in the simulator, but out of scope here. Overall updater statement coverage climbs from 64.5% to 77.0%: DownloadTarget 0% -> 59.5%, FindCachedTarget 0% -> 81.2%, GetTargetInfo 40% -> exercised more thoroughly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
The multirepo package was sitting at 14% statement coverage because the only methods exercised were NewConfig and validateRepoName -- the construction error paths and the standalone EnsurePathsExist were untested. Add two new tables: * TestEnsurePathsExistTable -- 5 cases mirroring the equivalent config_test.go table: happy path with fresh dirs, DisableLocalCache no-op, idempotency on already-existing dirs, os.ErrNotExist on empty paths, and the file-in-place-of-dir collision. * TestNewFailureCases -- two cases that fire before any per-repo updater is built: invalid root bytes that fail updater.New internally, and sabotaged TrustedRoots that hit the "trusted root missing" branch in initTUFClients. Net coverage: multirepo statement coverage climbs from 14.0% to 37.2%. EnsurePathsExist 0% -> 100%. New 57.1% -> 85.7%. initTUFClients 0% -> 76.0%. Refresh, GetTopLevelTargets, GetTargetInfo and DownloadTarget remain at 0% -- they require a working multi-repo TUF fixture, which is a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
The new TestGetTargetInfoTable / TestDownloadTargetTable / TestFindCachedTargetTable cases route through simulator.RepositorySimulator.fetch, which keys off filepath.Separator and the hardcoded "/targets/" prefix -- unreachable on windows where filepath.Separator is backslash. The underlying production code still gets coverage from the Linux and macOS runners. Skip the three tests on runtime.GOOS == "windows" until the simulator's target URL routing is fixed in a separate change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
…Tripper The original test passed http.DefaultTransport -- already the effective default Transport on http.DefaultClient -- so the assert.Same check would still pass if NewFetcherWithRoundTripper stopped installing the supplied transport. Reported by Copilot reviewer on #737. Switch to a one-off recordingRoundTripper local type that we can identity-compare against, and restore the original http.DefaultClient.Transport after the test (the constructor mutates the global as a side effect). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Two follow-ups from Copilot's review on #737: * TestGetTargetInfoTable claimed to exercise GetTargetInfo's implicit Refresh, but each subtest called createAndRefresh() first, so the inner Refresh branch was never reached. Switch to constructing the updater directly with New(cfg) and let GetTargetInfo trigger the refresh itself. * TestDownloadTargetTable picked which baseURL to pass by string- matching tc.name == "uses explicit baseURL argument", which is brittle (any rename of the subtest silently changes the routing). Replace with an explicit useExplicit bool on the case struct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
rdimitrov
force-pushed
the
config-coverage-followup
branch
from
June 3, 2026 15:19
123dcd5 to
0ea2003
Compare
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
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.
Stacked on top of #734.
Pushes statement coverage up across the packages with the largest gaps, plus a couple of hygiene cleanups. Six commits, one per concern.
Coverage moves
metadata/configSetDefaultFetcher*/SetRetryOptionsmethods (all 0%) now exercisedmetadata/fetcherDownloadFiletests now hermetic viahttptest.Serverinstead ofjku.github.iometadata/updaterGetTargetInfo,DownloadTarget,FindCachedTargetcoveredmetadata/multirepoEnsurePathsExist(0% → 100%),Newfailure paths,initTUFClients(0% → 76%)Commits
chore(testutils): drop the unused helpers/updater.goUpdaterTestCase,CheckError,AssertFilesExist,AssertFilesExact,TrustedMetadataTestCase,RunTrustedMetadataTestshad zero callers; the README pointed at functions that didn't even exist. Drop both.test(config): cover the SetDefaultFetcher* and SetRetryOptions methodsUpdaterConfigmethods. 28.6% → 97.1%.test(fetcher): cover the constructors and setterstest(fetcher): make DownloadFile tests hermetic via httptest.Serverjku.github.io/tuf-demoURLs with a localhttptest.Server. Removes external CI dependency. Adds aBodyExceedsLimitcase for the post-readLimitReaderpath. 87.8% → 93.9%.test(updater): cover GetTargetInfo, DownloadTarget and FindCachedTargetsimulator.TestRepository. The simulator has a couple of bugs in target URL routing (lastIndexpanics on shallow paths; hash-prefix parser confuses nested paths) — sidestepped with doubly-nested paths andconsistent_snapshot = false. Worth fixing in the simulator separately. 64.5% → 77.0%.test(multirepo): cover EnsurePathsExist and New's construction failuresEnsurePathsExist(mirrors the equivalent inmetadata/config) and theNew()construction error paths. 14.0% → 37.2%.Not done (intentionally)
multirepo.Refresh / GetTopLevelTargets / GetTargetInfo / DownloadTarget— still 0%. They need a working multi-repo TUF fixture (multiple simulators wired together via a map.json). Worth a follow-up PR.Merge order
If #734 merges first, the base auto-updates to master and this can merge cleanly. If #734 lands as something different, this branch rebases easily — none of the commits depend on the testing-rework rename pattern.
🤖 Generated with Claude Code