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_hours fall to its column default. It now writes 24 explicitly, and 24 is 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.
  • rtIntNewFixture now 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-ups section.

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.go named the seeding and composition helpers but not rpSHA256, rpETag and the row-count method the tarball suite now calls directly, and said "both" where the rdt* 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.
  • rpBinding writes its four fields with no defaulting, and its three zero values mean three different things. cacheHours: 0 is the silent one: it is the legitimate pin internal/datastore/npm_remote_freshness.go describes, the column CHECK admits it, and rtIntPinnedValidityHours drives 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, and url followed in 89f2b0bbd.
  • rpSeedRemoteRepository's doc now says it registers the cache-row sweep, which it stopped saying once the tarball fixture no longer called rtIntRegisterCacheCleanup by name.
  • rtIntNewFixture builds its upstream ahead of the seeding because the binding needs the URL, which also leaves the server's t.Cleanup last 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 is recordRemoteTarballDownload, through bufferedUpdate in internal/format/npm/buffered.go, bounded by bufferedUpdateTimeout and bufferedUpdateMaxInFlight. This bullet used to name internal/remote/standalone.go's BumpLastDownloadedAt, which the tarball route does not reach: remote.NewStandalone is built only by the packument and dist-tags routes.

89f2b0bbd applies the MR-review findings, comments only again:

  • rpBinding's zero-value list covered authToken, cacheHours and metadataHours but not url, so it read as complete when it was not. npm_remote_repositories.url is text NOT NULL with only a length CHECK, so an omitted url seeds 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 deleted rtIntRegisterCacheCleanup doc carried. That left the rule living only in the statement order of one function body: moving the call above seedNamespace puts 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.go named three stale entries, which reads as a complete correction, and this file also takes rpSeededRemote, rpBinding, rpMetadataValidityHours, rpSHA256 and rpETag from 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.
  • rtIntNewFixture said that building the upstream ahead of the seeding leaves "its close last to run". newTestClient registers client.Shutdown before rtIntNewFixture runs, 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.go lists what the file reuses from the test binary and names seedNamespace, seedNpmRepositoryKind and rpTxRunner. This MR removed the last use of all three; they now arrive through rpSeedRemoteRepository and rpNewOperations. 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.go calls its cap-breach test "the only caller of rdtNewFixture's maxPackumentBytes override". Which test sets the knob is still true, but the override now lives in rpNewFixture, as that file's own rdtNewFixture doc 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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading