test(install): compare stderr exactly and check every package.json and bun.lock row in bun-add-filter.test.ts

Closed
RO
1mo ago
+499-398

Problem

  • Most of the 124 cases in test/cli/install/bun-add-filter.test.ts check stderr with not.toContain("error:") and read 1 to 3 of the 5 package.json files. A stray warning, an unwritten lockfile, an edit to an unselected workspace or a bun.lock that does not match the files all pass.
  • The file was handed over as slow (20s to 22s on x64-asan). The CI logs say that time is not the cases (see the notes), so this PR claims no speedup. An earlier revision seeded each case's install cache. It measured within noise in CI and is gone.

Fix

  • run strips the two progress lines from stderr. The cases compare the rest exactly: Saved lockfile, nothing, or the exact warn, note or error text.
  • expectWorkspaces checks all 5 package.json files and the rows bun.lock keeps for them. expectAddedOnlyTo, used by the relation and selector cases, checks the same rows. installUnchanged pins that a plain install afterwards has nothing to save.
  • The outdated tables, the dry-run listings and three install summaries are pinned line by line. The "writes nothing" cases also check that bun.lock is absent or byte-identical.
  • Verified: bun bd test test/cli/install/bun-add-filter.test.ts, 123 pass, 1039 assertions against 997 before, in the same 216 processes. The previous revision passed every lane of build 101612 and 12 release runs on Windows x64. This one only removes setup.

Background

  • bun.lock repeats each workspace's name, version and dependency groups under workspaces, keyed by path. "" is the root. A filtered add or remove has to keep those rows in step with the files it edits.
  • bun install prints Resolving dependencies and Resolved, downloaded and extracted [N] to stderr when it downloads something, and Saved lockfile when it writes bun.lock. Only N varies between runs.
  • Three open PRs touch this file (#36284, #35461, #39232). The four places they rewrite are left byte-identical.
Notes

Where the 22s goes. The dots reporter flushes about once a second with a timestamp. On x64-asan the file prints its header at +0.1s, its first dots at +15.1s and its summary at +22.2s (build 101560, position 4 of 289 in its shard), or +13.9s and +20.6s (build 101586). With the cache seeding of the previous revision: +15.6s and +22.1s (build 101612). So the 124 cases take 6.5s to 7.1s, and the 14s in front of them is the first verdaccio start of the shard, under the ASAN build, while the job is still starting its containers (the coordinator: ... ready lines land in the middle of this file in all three builds). The next verdaccio file in the same shard, isolated-relink.test.ts, starts its registry and runs 7 cases in 3.3s. On darwin, where the file runs 16th, it takes 1.3s to 1.7s in total; at position 3 (build 101612, a PR build puts the touched file first) it takes 6.9s. The expected duration of this file is mostly the cost of being the first registry user in its shard. The levers for that are in test/harness.ts: the verdaccio child runs under bunExe() in CI (harness.ts:1905), stop() does not stop it (#36352), start() (#38891), or no verdaccio at all (#33115). The 6.5s to 7s that is this file's own is 216 ASAN process starts 5 at a time (src/options_types/context.rs:506).

The cache seeding that was removed. One install in beforeAll, its manifests and package directories copied into every test dir. With verdaccio under the debug ASAN build locally it looked large (the file went from 55s to 45s), but that registry is far slower than the release ASAN one CI runs: in CI the cases went from 6.7s and 7.1s to 6.5s, within the flush granularity, and with the registry under a release bun the debug build measured 12.3s to 12.7s before and after. Release runs measured 1.2s against 1.0s to 1.1s on Linux and 1.78s against 1.79s to 1.89s on Windows. The self-review also pointed out that #39741 is solving the same question differently for another file, so if seeding is wanted it belongs in VerdaccioRegistry, not in one file. Sharing one pre-installed tree for the 17 setup installs of GRAPH and ROOT_GRAPH would save about 0.5s of the 7s and is not done either.

Assertion changes.

  • 101 success sites go from not.toContain("error:") to toBe("Saved lockfile\n"). The sites where nothing may be written pin "": both --dry-run cases, the two --only-missing runs that find everything present, the two removes of an undeclared name, the second filtered install, --frozen-lockfile, and the four outdated runs. The sites with a warning or note pin the whole text and its order, including the postinstall case (Saved lockfile, then $ exit 1, then the error), which shows the files are written before the script runs.
  • About 20 error sites pin the whole stderr. They now check which patterns a no-match error lists, the note: line outside a project, the 404 line, the catalog refusals and the spelled-differently error. Errors with a code frame pin their own line instead of error:: unparsable root (failed to parse package.json ... ParserError), missing workspace dir, unsatisfied workspace: range with its Version: suffix, missing local path with the re-spelled name.
  • expectWorkspaces replaces partial checks at about 45 sites. It compares the whole workspaces section of bun.lock, so an extra or stale row fails too (a review comment asked for the key set). expectAddedOnlyTo covers the roughly 30 relation and selector cases; it compares the five known rows, because one of those cases adds a sixth workspace. The remove, --lockfile-only, catalog and --trust cases also check the packages, catalog and trustedDependencies sections of bun.lock. The !packages/web case checks the key set by hand instead, because web has no row there.
  • stdout pinned line by line: both outdated cases, both --dry-run cases (the listing shows the whole monorepo is resolved, not just the target), links only the selected workspace (2 packages installed), re-links only the selected workspace (1 package installed, which is also what an unfiltered remove prints when it has to link something), the --only-missing summary (4 packages installed), and the no-match remove banner.
  • installUnchanged replaces installOk and not.toContain("Saved lockfile") at 6 post-check sites. Six "writes nothing" cases also check that bun.lock is absent or byte-identical. In the pnpm#5601 case the second pattern is pinned as a no-op (""), which is what shows it selected the same workspace as the first one.
  • Left byte-identical: the -F ... -d -E case and the two-groups --peer case (#36284 rewrites both), the link: case (#35461), the skipIf line of the unwritable case (#39232), and the body of that case, which is skipped as root here. The imports, beforeAll, makeMonorepo and envFor are unchanged too. A new case only needs expect(stderr).toBe("Saved lockfile\n") and, where it applies, expectWorkspaces.
  • PROGRESS_LINES is the same constant #39646 adds to bun-update.test.ts. Moving it into harness.ts is a follow-up once both are in.

no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.