Finish the testing rework (#695 follow-up)#734
Open
rdimitrov wants to merge 22 commits into
Open
Conversation
rdimitrov
force-pushed
the
rework-testing-ci-fixes
branch
3 times, most recently
from
May 26, 2026 07:49
acaf103 to
be5eabd
Compare
rdimitrov
force-pushed
the
rework-testing-ci-fixes
branch
from
May 27, 2026 09:59
550514f to
dd5fbc9
Compare
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Remove TempDirManager (callers use t.TempDir()), RunTableTest/TestCase
generic wrappers, and the BenchmarkOperation abstraction. Replace with
a simpler flat set of functions that each call t.Helper() directly.
Key changes:
- Rename StripWhitespaces → StripWhitespace (singular)
- Rename NoError → AssertNoError, ErrorContains → AssertErrorContains
- Migrate FuzzDataGenerator from deprecated math/rand to math/rand/v2
with PCG source; change NewFuzzDataGenerator(seed int64) to
NewFuzzDataGenerator(seed1, seed2 uint64)
- Add Build{Root,Targets,Snapshot,Timestamp}JSON builder functions that
do not require *testing.T, eliminating the &testing.T{} anti-pattern
in fuzz seed corpus setup
- Keep CreateTest*JSON(t) as thin t.Helper wrappers for backward
compatibility
- Replace containsString/findSubstring reimplementation in updater.go
with strings.Contains from the standard library
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Co-authored-by: Claude <noreply@anthropic.com>
Callers of the rewritten helpers package required updates:
marshal_test.go:
- helpers.NoError → helpers.AssertNoError (3 call sites)
- SuccinctRoles BitLength 256 → 8 (256 is out-of-range, valid max is 32)
- Null_values wantErr false → true (null HexBytes fails to unmarshal)
metadata_fuzz_test.go:
- NewFuzzDataGenerator(time.Now().UnixNano()) →
NewFuzzDataGenerator(uint64(time.Now().UnixNano()), 0) (new two-arg API)
- helpers.CreateTestRootJSON(&testing.T{}) → helpers.BuildRootJSON()
(eliminates the &testing.T{} anti-pattern in f.Add seed setup)
metadata_bench_test.go:
- All for i := 0; i < b.N; i++ loops → for b.Loop() (Go 1.24+ API)
- math/rand → math/rand/v2 for generateRandomString helper
metadata_table_test.go:
- Replace TempDirManager with t.TempDir() (4 call sites)
config/config_table_test.go:
- Full rewrite: old tests had wrong assumptions about EnsurePathsExist
(uses os.MkdirAll — it creates dirs, not validates existence) and
about config.New (sets RemoteMetadataURL, not LocalMetadataDir)
- New table tests: TestUpdaterConfigNew, TestUpdaterConfigDefaults,
TestEnsurePathsExistTable, TestUpdaterConfigCopy,
TestUpdaterConfigCustomFetcher
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Co-authored-by: Claude <noreply@anthropic.com>
Add the client-under-test binary required by the tuf-conformance test suite (https://github.com/theupdateframework/tuf-conformance). cmd/tuf-conformance-client/main.go: Implements the three-command CLI protocol from CLIENT-CLI.md: - init <trusted-root> copies root.json into --metadata-dir; no network requests are made - refresh runs updater.Refresh() to update top-level metadata from --metadata-url - download refreshes metadata then downloads and verifies the artifact named by --target-name, caching if already present All diagnostic output goes to stderr; the process exits 0 on success and 1 on any error, satisfying the test suite's expectations. .github/workflows/conformance.yml: Runs the tuf-conformance GitHub Action on push, pull_request, and a weekly schedule (Wednesday 06:30 UTC) so the public conformance report stays current. Makefile: Add 'build-conformance-client' and 'conformance' targets for local development runs using a locally-installed tuf-conformance suite. Signed-off-by: Marvin Drees <marvin.drees@9elements.com> Co-authored-by: Claude <noreply@anthropic.com>
…ble tests Add infrastructure to make updater integration tests easier to write and read, then exercise it with a table-driven test file. simulator/builder.go: SimulatorBuilder is a fluent API for configuring a RepositorySimulator before a test starts. It covers the most common dimensions: consistent-snapshot flag, target files, delegations, succinct roles, expired/unsigned roles, version overrides, and root rotations. simulator/test_repository.go: TestRepository encapsulates all per-test state (simulator, metadata dir, targets dir, root bytes) in one struct. Temp dirs are created with t.TempDir() so they are cleaned up automatically; Cleanup() is a kept-for-compatibility no-op. Convenience methods cover the mutations and assertions most updater tests need. metadata/updater/updater_table_test.go: Table-driven tests for the TUF client workflow: root updates (version rollback, non-consecutive version, expired intermediate root), timestamp and snapshot unsigned/expired/version-mismatch scenarios, targets unsigned/expired/version-mismatch, fast-forward recovery for all three roles, version rollback detection, consistent-snapshot fetch sequence, and hash-mismatch detection. internal/testutils/README.md: Rewritten to document the current API (AssertNoError, AssertErrorContains, Build*JSON builders, FuzzDataGenerator, TestRepository, SimulatorBuilder) and remove references to removed types (TempDirManager, NoError, AssertEqual, RunTableTest). Signed-off-by: Marvin Drees <marvin.drees@9elements.com> Co-authored-by: Claude <noreply@anthropic.com>
Layers fixes for the CI failures observed on PR #695, plus a sync with master to incorporate intervening changes (#733 threshold fix and dep bumps). * fix(conformance-client): set LocalTargetsDir on refresh. Updater.New calls cfg.EnsurePathsExist on both LocalMetadataDir and LocalTargetsDir; an empty LocalTargetsDir made os.MkdirAll fail before any network work, which was the root cause of the broad "assert 1 == 0" failures across the conformance suite. * fix(conformance-client): honour libfaketime FAKETIME env. Go binaries on Linux read clock_gettime via VDSO and bypass libfaketime interception, so faketime-driven tests like test_faketime and the sigstore-root-signing static repository saw the real wall clock. Parse FAKETIME -- including Ubuntu's signed second-offset form (e.g. "+691200") -- and feed the parsed instant into Updater.UnsafeSetRefTime before Refresh. * test(config): drop the "not a directory" substring check in TestEnsurePathsExistTable; Windows os.MkdirAll returns "The system cannot find the path specified." expectError: true already covers the intent. * test(metadata): same OS-string fix for TestMetadataFromFile/Non-existent_file. Result: 108/108 tuf-conformance tests pass; CI green on linux/macOS/windows for both stable and oldstable Go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
config_test.go was already table-driven, just under a different file name. Its coverage is a subset of what was config_table_test.go, with three small gaps closed here: * TestUpdaterConfigDefaults now asserts LocalTrustedRoot equals the rootBytes passed to New, and RemoteTargetsURL is derived as RemoteMetadataURL + "/targets". Both were checked in the old TestNewUpdaterConfig via EqualExportedValues. * TestEnsurePathsExistTable gains an errorIs field, and the empty- paths case now asserts the returned error is errors.Is os.ErrNotExist (the stricter check the old TestEnsurePathsExist made). The old config_test.go is removed and config_table_test.go is renamed back to config_test.go now that there is only one test file per the project convention. go test -cover ./metadata/config/... stays at 28.6% of statements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Replace the 23 iterative tests in trustedmetadata_test.go with 5 table-driven tests grouped by Update* operation: * TestUpdateFlowTable -- sequenced happy path and ordering rules (covers the old TestUpdate and TestOutOfOrderOps). * TestUpdateRootTable -- UpdateRoot scenarios including invalid JSON, unsigned, wrong type, threshold bump, same version, plus the cross-cutting expired-final-root flow. * TestUpdateTimestampTable -- UpdateTimestamp scenarios including invalid input, version regression, equal version, snapshot meta regression, and expired timestamp blocking snapshot. * TestUpdateSnapshotTable -- UpdateSnapshot scenarios including invalid input, threshold failure, length mismatch, missing meta entries, meta version regression, expired snapshot blocking targets, plus the multi-step rollback flow as a dedicated subtest. * TestUpdateTargetsTable -- UpdateTargets scenarios including invalid input, missing snapshot meta, hash/length mismatch, version disagreement, and expired targets. Coverage holds at 92.8% of trustedmetadata statements; every old test scenario maps to a new subtest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Replace the iterative tests in updater_top_level_update_test.go (32) and updater_consistent_snapshot_test.go (4) with table-driven tests in updater_test.go, all written against the simulator.TestRepository builder so each subtest gets a fresh repo state. Tables and focused tests added: * TestUpdaterConstructorTable -- safe/unsafe constructors and the missing-trusted-root-and-URL rejection. * TestUnsafeRefreshTable -- fresh unsafe refresh fails (no fetches), warmed-cache unsafe refresh loads the cached set. * TestTrustedRootUnsignedTable -- locally-stored root with cleared signatures is rejected. * TestIntermediateRootIncorrectlySignedTable -- published root v2 not signed by trusted keys is rejected. * TestMaxRootRotationsTable -- the client honours cfg.MaxRootRotations. * TestTrustedRootExpiredRecoveryTable -- recovery after the expired-root failure works once a fresh root is published. * TestExpiredTimestampVersionRollbackTable -- rollback protection uses the local timestamp even when it has expired. * TestNewTimestampSnapshotRollbackTable -- a strictly newer timestamp whose snapshot meta regresses is rejected. * TestComputeMetafileHashesLengthTable -- enabling and disabling the simulator's hash/length computation across successive refreshes. * TestSnapshotRollbackWithLocalSnapshotHashMismatchTable -- rollback detection fires even when the local snapshot's hash disagrees with timestamp.meta. * TestExpiredMetadataTable -- expired local timestamp/snapshot can still drive a refresh from the remote. * TestMaxMetadataLengthsTable -- per-role MaxLength caps trip ErrDownloadLengthMismatch. * TestTimestampEqVersionsCheckTable -- timestamp with mutated meta but unchanged version does not replace the trusted copy. * TestDelegatesConsistentSnapshotTable -- delegate-fetch behaviour for both consistent-snapshot modes. After this commit, updater_top_level_update_test.go and updater_consistent_snapshot_test.go are removed and the table file becomes the canonical updater_test.go. Coverage holds at 64.8% of updater statements (up from 63.5% pre-conversion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Merge the three previous test files into the canonical metadata_test.go and table-ify the per-role clusters where the bodies share a template. File-level consolidation: * metadata_table_test.go (table-driven I/O coverage) + the existing metadata_api_test.go (API-level tests, TestMain) merge into metadata_test.go. No logic changes from this part -- pure textual concatenation with merged imports. In-file consolidation: * TestIsExpiredRoot/Snapshot/Timestamp/Targets (4) -> TestIsExpiredTable (1 test, 4 cases). * TestRootReadWriteReadCompare/Snapshot.../Targets.../Timestamp... (4) -> TestRoundtripFileTable (1 test, 4 cases) using a per-role closure that reads a fixture, writes it, re-reads, and compares. marshal_test.go, logger_test.go, metadata_bench_test.go, and metadata_fuzz_test.go remain as-is: they cover distinct concerns (JSON marshalling edge cases, logger plumbing, benchmarks, fuzz corpora) and grouping them into metadata_test.go would obscure their purpose without adding coverage. The other iterative-looking clusters (TestDefaultValues*, TestVerifyDelegateDuplicate*, TestVerifyLengthHashes*, TestUnrecognizedField*) are deliberately left iterative because the per-case bodies differ enough that a table would obscure rather than clarify. Net: 66 -> 60 Test* functions; coverage holds at 82.0% of metadata statements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
The repository_data/ tree holds PEM PKCS1 RSA keys signed with the rsassa-pss-sha256 scheme, a combination stock signing utilities do not handle. The little helper at internal/testutils/signer/signer.go was added in #625 specifically to bridge that, but only documented in its own inline comment -- meaning anyone who edits a fixture and watches verification fall over has to spelunk through internal/testutils/signer to find the fix. Add a "Static fixtures" + "Regenerating signatures" section to internal/testutils/README.md pointing at the tool with the canonical command line and the per-role key list, and steer contributors toward the in-memory simulator builders for new tests so the regeneration step is needed less often going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
rdimitrov
force-pushed
the
rework-testing-ci-fixes
branch
from
June 3, 2026 13:52
ab5f1cb to
c193205
Compare
rdimitrov
marked this pull request as ready for review
June 3, 2026 14:02
There was a problem hiding this comment.
Pull request overview
This PR completes the repository-wide testing rework by consolidating/rewriting tests into table-driven suites, introducing a new isolated simulator.TestRepository harness for updater tests, and adding tooling/docs/CI support for running the upstream tuf-conformance suite against this implementation.
Changes:
- Replace iterative updater tests with a consolidated, table-driven
metadata/updater/updater_test.gousing the newinternal/testutils/simulator.TestRepository. - Add new
metadatafuzzing and benchmarking suites plus additional JSON marshal/unmarshal tests. - Improve test utilities (
internal/testutils/helpers,internal/testutils/simulator), add a conformance client binary + GitHub Actions workflow, and refine error handling in multirepo/config tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
metadata/updater/updater_top_level_update_test.go |
Deleted legacy iterative updater top-level update tests (superseded by unified table-driven suite). |
metadata/updater/updater_consistent_snapshot_test.go |
Deleted legacy consistent-snapshot updater tests (coverage moved into unified suite). |
metadata/updater/updater_test.go |
Added consolidated table-driven updater tests using simulator.TestRepository. |
metadata/multirepo/multirepo.go |
Introduced typed sentinel errors and wrapped map-file unmarshal errors with context. |
metadata/multirepo/multirepo_test.go |
Added table-driven tests for NewConfig plus repo-name validation coverage. |
metadata/metadata_api_test.go |
Deleted legacy metadata API test file (tests reorganized elsewhere). |
metadata/metadata_fuzz_test.go |
Added fuzz tests for metadata parsing/serialization and related structures. |
metadata/metadata_bench_test.go |
Added benchmarks for metadata creation, (de)serialization, and concurrency patterns. |
metadata/marshal_test.go |
Added JSON marshal/unmarshal and error-case tests for metadata types. |
metadata/config/config_test.go |
Reworked config tests into table-driven suites and expanded EnsurePathsExist coverage. |
Makefile |
Added conformance target and a helper target to build the conformance client. |
internal/testutils/simulator/util.go |
Introduced shared simulator helpers (key generation + path utilities). |
internal/testutils/simulator/test_repository.go |
Added TestRepository harness for isolated, non-global simulator state and common assertions. |
internal/testutils/simulator/repository_simulator.go |
Refactored simulator internals to use shared helpers and removed duplicated path/key helpers. |
internal/testutils/simulator/repository_simulator_setup.go |
Changed InitMetadataDir to return errors instead of os.Exit(1) on init failures. |
internal/testutils/simulator/config.go |
Added simulator options plumbing (WithDelegates, etc.). |
internal/testutils/simulator/builder.go |
Added fluent SimulatorBuilder to construct preconfigured simulators for tests. |
internal/testutils/helpers/helpers.go |
Added reusable test helper utilities and JSON fixture builders (including fuzz-safe builders). |
internal/testutils/helpers/fuzz.go |
Added deterministic fuzz data generator utilities and a helper to register common fuzz seeds. |
internal/testutils/helpers/updater.go |
Added shared table-test scaffolding for updater/trusted-metadata style tests. |
internal/testutils/README.md |
Documented the new testutils structure and fixture regeneration procedure. |
cmd/tuf-conformance-client/main.go |
Added the conformance client-under-test CLI (init/refresh/download protocol). |
.github/workflows/conformance.yml |
Added CI workflow to run the upstream tuf-conformance suite against the client binary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NewConfig previously wrapped both ErrNoMapFile and ErrNoTrustedRoots in a single "%w and/or %w" message even when only one of the inputs was missing. That made errors.Is(err, ErrNoMapFile) return true even when the caller had passed a valid map file but no trusted roots, which is misleading and prevents callers from using errors.Is to discriminate. Reported by Copilot reviewer on #734. Split the validation into three branches that wrap only the sentinel(s) matching the actually-missing input. Add a new TestNewConfigErrorWrapping test that asserts each combination produces the expected errors.Is / NotErrorIs behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Per Copilot review on #734: seeding the fuzz data generator with time.Now().UnixNano() and using time.Now() in the fuzz-generated payload makes the initial corpus non-reproducible across runs -- a fuzz crash can be hard to replay because the inputs that were fed to f.Add change each invocation. * Replace all uint64(time.Now().UnixNano()) seeds in metadata_fuzz_test.go with the constant 0. * Replace the time.Now() expiry inside CreateFuzzTestMetadata's generated payload with a fixed RFC3339 timestamp. Net: 9 seed sites switched to 0, one timestamp pinned to 2030-01-01T00:00:00Z, and the now-unused "time" import dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
GetUpdaterConfig previously fed r.RootBytes -- the snapshot captured at TestRepository init time -- into config.New on every call. After a successful Updater.Refresh persists an updated root.json to MetadataDir, the next GetUpdaterConfig + Updater.New pair would feed the *stale* initial bytes back in, silently rolling the on-disk root.json back to its initial version and breaking the notion of a warmed cache for tests that build a second updater (e.g. the unsafe-cached-refresh path in TestUnsafeRefreshTable). Reported by Copilot reviewer on #734. Read root.json from r.MetadataDir on each invocation instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
The function uses strings.HasSuffix but its parameter was named "prefix", which is misleading at call sites. Reported by Copilot reviewer on #734. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
…etadata package The fuzz body in CreateFuzzTestMetadata used a local variable named "metadata" -- the same identifier as the enclosing package, which shadows it within the closure and makes the surrounding Root()/Targets() calls harder to read. Reported by Copilot reviewer on #734. Rename to "payload". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.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
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.
Picks up Marvin's testing-rework draft (#695) and finishes it: green CI, table-driven tests across every production package, and docs for the fixture regeneration step that wasn't written down anywhere.
What's in the PR
e1aad84LocalTargetsDirbug in the conformance client,$FAKETIMEenv parsing (Go binaries don't honourLD_PRELOADlibfaketime), two Windows-incompatible error-string asserts.7a36600metadata/config/: consolidate iterative tests into one table-driven file.7a739ebmetadata/trustedmetadata/: 23 iterative tests → 5 tables.969b0d8metadata/updater/: 36 iterative tests across 2 files → 24 tests in one file, all on the newsimulator.TestRepositorybuilder.e1e45eemetadata/: mergemetadata_test.go+metadata_api_test.go+metadata_table_test.gointo one file; collapse 8 per-role iterative tests into 2 tables.c193205rsassa-pss-sha256scheme viainternal/testutils/signer).Plus #695's 11 commits, preserved with Marvin as author.
Results
*_test.gofile — no*_table_test.gostaging files leftCI status
Everything green except
linting / govulncheck, which is also red on master right now: two new Go stdlib CVEs (GO-2026-5039,GO-2026-5037) fixed in Go 1.26.4. Needs a master-level toolchain bump; not specific to this PR.cc @MDr164
🤖 Generated with Claude Code