Repository navigation
test(producer): compiler parity test runs without the network - #5087
Conversation
somanshreddy
left a comment
There was a problem hiding this comment.
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 13getaddrinfo EAI_AGAIN cdn.exampleerrors and connection attempts tolocalhost:3000(::1and127.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
disableCSSFileLoadingand happy-dom goes back to loading the<link>stylesheets: 10cdn.examplelookups pluslocalhost:3000connects. - Turn off
handleDisabledFileLoadingAsSuccessand 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.
- Turn off
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
left a comment
There was a problem hiding this comment.
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.mockremoved (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 changesandSemantic PR titlewere 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.tsxand the sidebar tests setdisableIframePageLoadingthe same way). The mock spreadsimportOriginalinstead of hand-stubbing the module. - Simplicity: 17 test-only lines and no production change. Nothing to cut.
— Rames
Edit accuracy: accurate 2055 (base branch 2055), smooth 1678 of thoseThe gate passes. Quarantined, measured but not gated (0) |
What
htmlCompiler.parity.test.ts > keeps each composition's link to a shared file under its own condition in preview, render and mountfailed 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:fetchPublicHttpsText, 15 s timeout) and keeps the<link>when that fails. Every run did a real DNS lookup forcdn.example.DOMParser, and happy-dom loads<link rel="stylesheet">even in a parsed document. That is the same lookup again, plushttp://localhost:3000/<file>.cssfor relative links, since happy-dom's page URL islocalhost:3000. That produced theECONNREFUSED 127.0.0.1:3000noise in the log.When the lookup is slow, the test runs past its 5 s limit.
How
In the test file only:
fetchPublicHttpsTextis mocked to refuse at once. The compiler then keeps the link tag, which is what the test asserts.No production code changes.
Test plan
dns.lookupthat 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.ENOTFOUND cdn.example,ECONNREFUSED localhost:3000); with this change it logs no network or file-loading errors (only the fixtures' own expected warnings).