Skip to content

test(producer): compiler parity test runs without the network - #5087

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/producer-parity-test-offline
Oct 6, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/producer-parity-test-offline

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

htmlCompiler.parity.test.ts > keeps each composition's link to a shared file under its own condition in preview, render and mount failed CI with "Test timed out in 5000ms" on a PR that does not touch the producer. The test reached out to the network, so its run time depended on the runner's DNS. It now runs with no network at all.

Why

The test links https://cdn.example/shared.css. Two things fetched it for real:

  • The render compiler downloads external stylesheets (fetchPublicHttpsText, 15 s timeout) and keeps the <link> when that fails. Every run did a real DNS lookup for cdn.example.
  • The test reads compiled output back with DOMParser, and happy-dom loads <link rel="stylesheet"> even in a parsed document. That is the same lookup again, plus http://localhost:3000/<file>.css for relative links, since happy-dom's page URL is localhost:3000. That produced the ECONNREFUSED 127.0.0.1:3000 noise in the log.

When the lookup is slow, the test runs past its 5 s limit.

How

In the test file only:

  • The compiler's fetchPublicHttpsText is mocked to refuse at once. The compiler then keeps the link tag, which is what the test asserts.
  • happy-dom's CSS file loading is turned off for this file, and a skipped file counts as loaded so happy-dom does not log an error for each link, in the same way other test files turn off iframe loading.

No production code changes.

Test plan

  • Red first: with every non-local DNS lookup made to hang (a preloaded dns.lookup that never answers, like a slow resolver), the test on main fails exactly as in CI ("Test timed out in 5000ms", 12 of 13 passing), and with this change all 13 pass.
  • A normal run on main logs 51 network errors (ENOTFOUND cdn.example, ECONNREFUSED localhost:3000); with this change it logs no network or file-loading errors (only the fixtures' own expected warnings).
  • 13 of 13 pass, three runs in a row. Format, lint and typecheck.

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 5, 2026 20:55

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at f5597fd8 (full PR, 2 commits, test-only). This is a comment, not an approval.

Verdict: no blockers. The change does what it says: the parity suite no longer reaches the network or a local server.

Evidence (run with no network at all, via unshare -rn, after building core):

  • Head: 13/13 pass, with no network errors and no warn/error log lines.
  • Parent of the PR (6c353d89), same offline run: 13/13 still pass, but it logs 13 getaddrinfo EAI_AGAIN cdn.example errors and connection attempts to localhost:3000 (::1 and 127.0.0.1). That matches the claim: the old run depended on DNS and connect timing rather than on the assertions.
  • Each setting is load-bearing:
    • Turn off disableCSSFileLoading and happy-dom goes back to loading the <link> stylesheets: 10 cdn.example lookups plus localhost:3000 connects.
    • Turn off handleDisabledFileLoadingAsSuccess and 12 error/warn lines come back. This confirms the second commit ("without error logs").
    • Both mutants still pass, so the suite doesn't fail if the isolation regresses; it only gets noisy or slow again. That's acceptable for test hygiene.

Low / note: the mock fixes which branch the remote-stylesheet test exercises. "keeps each composition's link to a shared file under its own condition in preview, render and mount" only holds when the external stylesheet fetch fails. If fetchPublicHttpsText returns CSS containing an @font-face, render inlines the fonts and drops the <link>, and the test fails (expected [] to deeply equal [{ media: null, title: 'alt' }, …]). Plain CSS without font-faces still passes. The suite was already effectively on this branch, since cdn.example never resolves, so this isn't a regression. Consider adding a one-line note on the mock (or in the test name) that it pins the "fetch failed, link preserved" path, so the successful-fetch branch isn't assumed to be covered here.

CI at this head: 4 pass, 7 pending (Actions outage), 2 skipping. No reviews on GitHub yet when I posted.

What I ran: read the diff; ran bunx vitest run src/services/htmlCompiler.parity.test.ts at the head, at the parent and under 4 mutants (the two happy-dom flags, plus the mock returning plain CSS and returning CSS with an @font-face), each offline under unshare -rn.

— Somu

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at f5597fd8. Somu's review already covers the offline run and the two happy-dom settings. This adds the slow-resolver case from the PR body, plus a check of which of the three changes matters there.

I reproduced the CI timeout, then the fix. I preloaded a dns.lookup that never answers for anything but localhost (NODE_OPTIONS=--import), which is how a slow runner resolver behaves:

  • Base test file: 12/13 pass, and "keeps each composition's link to a shared file…" fails with Test timed out in 5000ms, the same as CI.
  • This head: 13/13. A normal run at this head is also 13/13.
  • Head with only the vi.mock removed (happy-dom settings kept): the same test times out again.

So under a slow resolver, the fetchPublicHttpsText mock is the change that fixes the timeout. The happy-dom settings remove the localhost:3000 and cdn.example noise, as Somu showed. The mock sits at the right point: htmlCompiler.ts imports fetchPublicHttpsText from that same ../utils/urlDownloader.js, and importOriginal keeps the module's other exports real (isHttpUrl, used by animatedGifPrep). The run-test-lane classifier puts this file on the vitest runner because it imports vitest, so vi.mock and window.happyDOM both apply.

CI hasn't run at this head. The 14 red checks are knock-ons from the Actions outage:

  • Detect changes and Semantic PR title were cancelled with no steps.
  • The gate jobs (Test, Typecheck, Build, regression, Windows) fail within seconds at "Require change detection", before any test runs.

Branch protection still needs these to pass on a rerun.

James's lens

  • Reuse: Object.assign(window.happyDOM.settings, …) follows the existing studio pattern (pointerTargetSize.test.tsx and the sidebar tests set disableIframePageLoading the same way). The mock spreads importOriginal instead of hand-stubbing the module.
  • Simplicity: 17 test-only lines and no production change. Nothing to cut.

— Rames

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Edit accuracy: accurate 2055 (base branch 2055), smooth 1678 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit f80614c Oct 6, 2026
122 of 136 checks passed
@miguel-heygen
miguel-heygen deleted the fix/producer-parity-test-offline branch October 6, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants