test(npm): extract the shared npm remote integration fixture harness
What this does
internal/format/npm compiles several remote-repository integration suites into
one test binary, and three of them each built their own fixture: the packument
suite (rp*), the dist-tags suite (rdt*), and the tarball suite (rtInt*).
The tarball one was the newest and was largely a copy of helpers already in the
binary, including a second copy of the npm_remote_* cleanup sweep. Two copies
of that sweep means a table added to the npm remote cache schema has to be
remembered twice, and a miss leaves rows behind that then fail some later,
unrelated test.
Issue #788 (closed) counts three copies of that sweep, and this description counts two.
The two is the corrected figure, and the correction came out of this work: at
the merge base rdtNewFixture had no sweep of its own and reached one through
rpNewFixture, so only the packument harness and the tarball suite held one.
This MR is test-only. No production code changes.
What is shared now
| New or widened helper | Replaces |
|---|---|
rpSeededRemote, embedded by both fixtures |
the db, ns, slug and res fields each declared |
rpSeedRemoteRepository |
the namespace, repository and binding seeding each did inline |
rpBinding passed to rpSeedRemoteBinding |
rtIntSeedRemoteBinding |
the single rpRegisterCacheCleanup sweep |
rtIntRegisterCacheCleanup |
rpNewOperations |
the cache stores, flight registry and provider the tarball fixture built inline |
rpSHA256, rpETag, rpSeededRemote.count |
rtIntSHA256, rtIntETag, rtIntFixture.count, rpCount |
rpFixture.deps |
the three config defaults rdtNewFixture re-derived by hand |
Embedding rpSeededRemote is what keeps the call sites still: every existing
f.db, f.ns, f.slug and f.res reads through it unchanged.
Why this is behavior preserving
Three differences, all deliberate. The first two are equivalent to what was there. The third is not, and it is safe under a condition this MR states:
- The tarball fixture used to let
metadata_cache_validity_hoursfall to its column default. It now writes24explicitly, and24is that default (internal/datastore/migrations/structure.sql). - The shared cleanup sweep also deletes from
npm_remote_metadata_files. A tarball fixture's namespace never holds such a row, so that is a no-op there. rtIntNewFixturenow builds its upstream ahead of the seeding rather than between the repository and the binding, because the shared seeder writes the binding and needs the URL. Under LIFO that moves the server's close from third in the unwind to last, so the request drain no longer sits ahead of the repository and namespace deletes. Safe as this suite is written, because no fill outlives its response. The condition is recorded at the call site and in the## Review follow-upssection.
Proof, rather than argument: go test -tags=integration -v ./internal/format/npm/
run before and after lists the same 1901 tests and subtests, and diffing the two
sorted inventories is empty. Nothing was dropped, renamed or quietly skipped.
Checks run locally
go build ./...,go vet ./...,go vet -tags=integration ./internal/...: clean.golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/npm/: 67 findings in the package, none of them in the four files this MR touches.scripts/ci/check-comment-caps.sh: clean.- The npm integration suite itself, green both before and after.
Size
457 reviewable lines, 170 added and 287 removed, so this deletes 117 lines net and sits under the 500-line threshold. Split by file:
| File | Added | Removed |
|---|---|---|
remote_packument_harness_integration_test.go |
124 | 71 |
remote_tarball_integration_test.go |
36 | 175 |
remote_disttags_integration_test.go |
6 | 37 |
remote_packument_integration_test.go |
4 | 4 |
Overlap with open merge requests
No open MR touches any of these four files. The two nearby ones are !1941 (merged)
(remote_packument.go) and !1807 (merged) (remote_tarball.go), both production files
this MR leaves alone.
Review follow-ups
170d12a2f applies the branch-review findings. Comments only: no test, no
assertion and no seeded value changed, so the inventory proof above still holds.
- The shared-symbol note in
remote_packument_harness_integration_test.gonamed the seeding and composition helpers but notrpSHA256,rpETagand the row-count method the tarball suite now calls directly, and said "both" where therdt*dist-tags fixture is a third consumer through its embedded*rpFixture. It now names the category and both dependent suites, so it needs no edit per helper. rpBindingwrites its four fields with no defaulting, and its three zero values mean three different things.cacheHours: 0is the silent one: it is the legitimate pininternal/datastore/npm_remote_freshness.godescribes, the column CHECK admits it, andrtIntPinnedValidityHoursdrives it today, so an omitted window would pin a fixture fresh and leave its freshness assertions vacuous. Each of those three fields now documents what its zero does, andurlfollowed in89f2b0bbd.rpSeedRemoteRepository's doc now says it registers the cache-row sweep, which it stopped saying once the tarball fixture no longer calledrtIntRegisterCacheCleanupby name.rtIntNewFixturebuilds its upstream ahead of the seeding because the binding needs the URL, which also leaves the server'st.Cleanuplast in the LIFO unwind. That is recorded with the condition that keeps it safe: no fill outlives its response, and the one detached write is an UPDATE, so it cannot trip the FK constraints the sweep order protects. That write isrecordRemoteTarballDownload, throughbufferedUpdateininternal/format/npm/buffered.go, bounded bybufferedUpdateTimeoutandbufferedUpdateMaxInFlight. This bullet used to nameinternal/remote/standalone.go'sBumpLastDownloadedAt, which the tarball route does not reach:remote.NewStandaloneis built only by the packument and dist-tags routes.
89f2b0bbd applies the MR-review findings, comments only again:
rpBinding's zero-value list coveredauthToken,cacheHoursandmetadataHoursbut noturl, so it read as complete when it was not.npm_remote_repositories.urlistext NOT NULLwith only a length CHECK, so an omittedurlseeds and the failure appears at the fetch instead of at the seed. Every field now has its line.rpSeedRemoteRepository's doc said it registers the cache-row sweep but not that the registration has to come after the seeders, which is the rule the deletedrtIntRegisterCacheCleanupdoc carried. That left the rule living only in the statement order of one function body: moving the call aboveseedNamespaceputs the namespace and repository deletes ahead of the sweep, and the symptom is a later, unrelated test failing on rows it did not create. The doc carries the rule again.- The two-line note under the header of
remote_tarball_integration_test.gonamed three stale entries, which reads as a complete correction, and this file also takesrpSeededRemote,rpBinding,rpMetadataValidityHours,rpSHA256andrpETagfrom the harness. The header carries a second claim as well, that no name in its list may be redeclared here, and that one had not gone stale while the single word "stale" covered both. The reword names the category and keeps the redeclaration rule. rtIntNewFixturesaid that building the upstream ahead of the seeding leaves "its close last to run".newTestClientregistersclient.ShutdownbeforertIntNewFixtureruns, so that close is last of this fixture's own cleanups, not last of the test's.
Every reworded block stays inside the 2-line cap a touched block in a
_test.go file takes, so scripts/ci/check-comment-caps.sh --base origin/main
returns OK.
Two things deliberately left alone
Both are comments made inaccurate by this MR that scripts/ci/check-comment-caps.sh
will not let us correct in place. It caps a touched comment block in a _test.go
file at 2 lines and charges the whole block, with no waiver by design, so the
only compliant edit is to compress the block to 2 lines and delete the rest.
- The file header of
remote_tarball_integration_test.golists what the file reuses from the test binary and namesseedNamespace,seedNpmRepositoryKindandrpTxRunner. This MR removed the last use of all three; they now arrive throughrpSeedRemoteRepositoryandrpNewOperations. Editing that sentence reports the block at 40 lines against the cap of 2, so the adjacent two-line note records that the list is neither current nor complete instead. remote_disttags_integration_test.gocalls its cap-breach test "the only caller ofrdtNewFixture'smaxPackumentBytesoverride". Which test sets the knob is still true, but the override now lives inrpNewFixture, as that file's ownrdtNewFixturedoc says. Correcting it reports 18 lines against the cap of 2, and the block carries the leader/follower split, the Retry-After rationale and the barrier note, so compressing it would cost more than the misattribution does.
Worth a separate decision rather than a per-MR judgement: a refactor that moves a symbol cited from a long legacy comment block must either leave the citation false or absorb an unrelated compression, and the first option has no ratchet.
Related to #788 (closed)