fix(server): stop pinning a yanked version in the vendor CDN test - #1219
Conversation
vivek7405
left a comment
There was a problem hiding this comment.
Read this one back with fresh eyes and it was worse than I thought when I pushed it. The mock half is solid: I traced it against jspmGenerate and a per-package revert really does make lint's isolated call win last-write, so that assertion earns its place. The live half was the problem. I swapped a fixture that had gone unresolvable for one that could not disagree with itself, which reads like coverage and is not.
The most useful catch is the flattenScope one, because nothing was red. Every mock in this file answers on install alone, so a mocked assertion on a transitive key just reads back a value the mock invented. The old live test was the only thing holding that flag honest, and picocolors plus clsx have no dependencies at all, so deleting flattenScope: true from vendor.js left the whole suite green. That is a coverage hole I opened while claiming to close one.
Fixed by picking the fixture for what it can catch rather than for being small: @codemirror/view@6.39.0 plus @codemirror/lint@6.9.5. They share a transitive that resolves differently per strategy (lint alone pulls view@6.41.1, the unified graph keeps the pinned 6.39.0) and hoist five transitives to top level. Both counterfactuals now fire, and I ran the skip paths too rather than assuming the guard sat in the right place.
Fixed in aa7d0f6d.
The #446 regression test pinned @codemirror/lint@6.9.6 and asked the live jspm.io to resolve it. That version stopped resolving upstream (the CDN answers "Module not found .../dist/index.js"), so the test failed on every run and blocked main for every PR, none of which had touched vendor code. Repinning to a newer version does not work. Only lint 6.9.6 and 6.9.7 carry the ^6.42.0 view range that creates the version CONFLICT the old fixture needed, and neither resolves on jspm.io today: 6.9.6 is broken and 6.9.7 is not mirrored. So the conflict shape has no live fixture left. Split the test in two along what each half can actually prove. The conflict shape moves to a deterministic mock. It answers a unified call with the coherent graph and a per-package call with the skewed view that install alone implies, so reverting jspmGenerate to the old per-package loop makes lint's isolated call hand back view@6.43.0, which wins last-write and reds the assertion. That invariant caught the shipped bug and no longer depends on a third party hosting one exact version. The ordering test moved to a mock for the same reason in #312. The live half keeps what a mock cannot give, that our merged output equals jspm's OWN unified graph, with a fixture chosen so the comparison can still fail. @codemirror/view@6.39.0 plus @codemirror/lint@6.9.5 share a transitive that resolves differently per strategy (lint alone drags in view@6.41.1, the unified graph keeps the pinned 6.39.0), and the pair hoists five transitives to top level, which is the suite's only coverage of the flattenScope flag: every mock here answers on `install` alone, so a mocked assertion on a transitive key would just read a value the mock fabricated. A fixture without a shared transitive catches neither. Upstream trouble skips instead of redding, but only upstream trouble, and the two are separated at the TRANSPORT rather than by reading the result. jspmGenerate fail-opens two different ways, a non-empty skewed per-install merge on a transient and {} on an unresolvable set, so no test of its return value can tell an upstream blip from our own bug: check for empty and a transient reds the build, check for skewed and a real regression hides. So the test watches what the network did. A throw, a 5xx or a 429 on our own call skips, and a 4xx does NOT, because the ground truth just succeeded for the same fixture, which makes a 4xx evidence that OUR request is malformed. Both live calls carry a timeout, since node --test sets no per-test deadline and a wedged endpoint would otherwise hold the job open to the CI limit.
aa7d0f6 to
0cb8e19
Compare
vivek7405
left a comment
There was a problem hiding this comment.
Went back over the reworked live test. The fixture change holds up: I re-checked both failure modes against the live API and a per-package revert really does diverge on @codemirror/view, while dropping flattenScope moves five keys out of imports into scopes. That part is doing its job.
The skip logic was not. I had it reading jspmGenerate's RETURN VALUE to decide whether upstream had flaked, and that cannot work, because the function fail-opens two different ways: a transient on the unified call gives back a non-empty per-install merge, and an unresolvable set gives back {}. Testing for empty means a transient still reds the build; testing for skewed means a real regression hides. I picked the first and got both halves of the problem, a blip that still blocks and a corrupted-request bug that skips silently.
Moved the decision to the transport. The test now watches what the network actually did on our own call and skips only on a throw, a 5xx or a 429. A 4xx deliberately does not skip: the ground truth succeeded for the same fixture moments earlier, so upstream is demonstrably fine and a 4xx means our request is the thing that changed. Verified both directions rather than reasoning about them.
Also gave both live calls a timeout. node --test sets no per-test deadline, so a wedged endpoint would have held the unit job open until the CI limit, which the shipped code already guards against in its own call and the test did not.
Fixed in 0cb8e193.
Closes #1218
mainwas red and every PR was blocked. The#446regression test pinned@codemirror/lint@6.9.6and asked the liveapi.jspm.ioto resolve it. That version stopped resolving upstream, so the test failed on every run, on PRs that had never touched vendor code.Repinning does not work, which changed the fix
The obvious move is to bump the pinned version. Checking the registry first says otherwise. The fixture needs a version conflict to be meaningful:
@codemirror/viewis pinned at6.39.0, andlinthas to transitively want something newer.@codemirror/lint@codemirror/viewrangeview@6.39.0?^6.0.0^6.35.0^6.42.0Module not found .../dist/index.js)^6.42.0Only
6.9.6and6.9.7create the conflict at all, and neither resolves. Every older version wants^6.35.0, which6.39.0already satisfies, so repinning to one of those would leave a green test that proves nothing: the discriminating assertion would pass because there was never a conflict, not because the unified call resolved one. There is no live fixture left that expresses this shape.So the test splits along what each half can actually prove
The conflict shape moves to a deterministic mock. The mock answers a unified call (both installs together) with the coherent graph, and a per-package call with the skewed
viewthat install alone implies. That asymmetry is what makes it discriminating: revertjspmGenerateto the pre-#446 per-package loop andlint's isolated call hands backview@6.43.0, which wins last-write and reds the assertion. This is the invariant that caught the shipped bug, and it no longer depends on a third party continuing to host one exact version.There is precedent in this same file: the install-ordering test was moved onto a mock for exactly this reason in #312, after the live CDN flaked on transitive resolution.
The live half keeps what a mock genuinely cannot give, that our merged output equals jspm's own unified graph. A mock only ever returns what the test file already believes, so it cannot verify our merge against reality.
Its fixture is picked so that comparison can still fail two distinct ways, because a parity assertion over a set with nothing to disagree about is decoration:
@codemirror/lint@6.9.5resolved alone drags inview@6.41.1; in the unified graph the pinnedview@6.39.0wins. A revert to the per-package loop diverges from the ground truth. A pair with no shared transitive (picocolors + clsx) cannot catch this, since its unified graph is byte-identical to the union of its single-install graphs.flattenScope. The pair hoists five transitives to top level.vendor.jssendsflattenScope: trueso the browser gets no unresolved bare specifier, and this is the ONLY coverage of that flag in the suite: every mock here answers oninstallalone, so a mocked assertion on a transitive key would just read a value the mock fabricated.lintis pinned at6.9.5rather than a version whoseviewrange excludes6.39.0, since only6.9.6and6.9.7do that and neither resolves. The incompatible-range case is the mock's job; the live half only needs a shared transitive that resolves differently per strategy.Upstream failure reaches a loud skip rather than a red, through every mode rather than only an
errorin a well-formed JSON body:fetchthrowing on DNS or a reset,.json()throwing on a proxy's HTML 502, and our own call fail-opening to an empty map. A red there means a third party changed something, which is not a signal about this repo and must not blockmain.packages/server/src/vendor.jsis unchanged, as the diagnosis predicted. This was a broken test fixture, not a framework bug.Test plan
node --test packages/server/test/vendor/vendor.test.js: 115/115.aa7d0f6d.jspmGenerateto the pre-dogfood: importmap per-package resolution yields inconsistent dep graph #446 per-package loop: reds the mock test AND the live parity test, 6 failures.flattenScope: trueinpackages/server/src/vendor.js: reds the live parity test. This is the finding that the previous fixture had silently stopped covering.TypeError: fetch failed), 0 failures.SyntaxError: Unexpected token '<' ...), 0 failures.test/bun/listener.test.mjs("the trusted socket IP must reach clientIp in the WS handler; got_anon_"), which fails identically onmainin a clean checkout under local Node 26.1.0. Pre-existing and unrelated; the repo targets Node 24+ and CI runs that.Doc surfaces
AGENTS.md, the skill references, docs site, MCP, editor plugins, scaffold templates, marketing copy: N/A, no public surface changed. This is a test-only diff.