Fix smoke-all dropping tests after a failed project pre-render - #14926
Merged
Merged
Conversation
…e-all tests/smoke/smoke-all.test.ts pre-renders a project (for files annotated render-project: true) in a bare, unguarded top-level await. A failing pre-render throws uncaught at module-evaluation time. Deno reports this loudly (exit 1, named error, real stack trace), but every Deno.test() that would have been registered after the throw - the rest of the glob, and everything after the loop - is silently never created and never counted. The summary reads "N passed | 1 failed" with no indication that M tests never ran at all. This is pre-existing and mode-independent (dev and binary mode both throw identically for an ordinary render failure); it is not a regression from binary-mode test support. render-project is per-file front matter, not a project-level setting, so a single-pass try/catch cannot isolate a failure: an unannotated sibling file can already be registered before the annotated file triggers the pre-render. Split the discovery loop into three passes instead - collect every file first, pre-render each distinct project once, then register tests - so a failed project's files can be skipped as a group (one named synthetic failure per project, not a crash of the whole file) while every other file, including ones after it in the file list, keeps running and counting normally. A skipped file's would-be cleanup entry is synthesized so its outputs are still removed at parity with a registered file. A timeout is only safe to isolate if the render is known to have stopped. Dev mode can never confirm this (no in-process cancellation), so its timeouts stay fatal. Binary mode's process-tree kill previously discarded its outcome entirely - the Windows path lost taskkill's exit code in a local that never escaped, and the Unix path treated any non-throwing pgrep call as an authoritative enumeration even though a non-zero exit (pgrep's own usage/internal error codes) produces the same empty output as a childless leaf. Made cancellation a total exit-code partition with a fail-safe default, carried through a new QuartoTimeoutError (renderCancelled, killDetail) instead of a bare string, so an unconfirmed kill can no longer be mistaken for a confirmed one and isolated anyway. The new tests/unit/ regression test spawns a nested deno test of smoke-all.test.ts against a generated fixture (an unannotated file excluded from the project's render list, two annotated files so a retry-per-annotation implementation can't pass trivially, and a trailing healthy file) and asserts on its JUnit report: exactly one synthetic failure, zero tests for any file of the broken project, and the trailing file still running - the exact coverage silently lost today.
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…tree race A registered file's teardown sweeps postRenderCleanupFiles (custom paths a testSpec declares via postRenderCleanup), but a file skipped because its project's pre-render failed never runs teardown. If a project's pre-render partially succeeds before failing (rendering some of its own files as a side effect) and none of the invocation's other files happen to run a normal teardown, those custom paths were never swept. Sweep once in the skip branch instead, after pass 1.5 has already run so any such artifact already exists. Documented, rather than attempted to close, the inherent TOCTOU gap in the Unix process-tree kill: the pgrep enumeration and the kill pass are not atomic, so a descendant spawned or reparented in between is invisible to the walk even when cancelled comes back true. Closing that gap needs a process group/job object, out of scope for test infrastructure and already weighed and rejected during design.
…ed path postRenderCleanup() swept every currently-registered custom cleanup path regardless of which file registered it. That's safe at a normal per-file teardown, since the suite runs one file at a time so nothing else has created a matching artifact yet by the time that teardown fires. The skip-branch sweep added for a failed project runs at a different point in time: after pass 1.5, when every project's pre-render (not just the failed one) has already completed, so an unscoped sweep there could delete a different, healthy project's artifact before that project's own tests get to verify it. Track which input file registered each cleanup path and scope the skip-branch sweep to that one file.
A skipped file must register the same cleanup entries its teardown would have, including never registering one for editor-support-crossref. Keeping that rule in one helper makes the parity structural instead of relying on two copies staying in sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a project pre-render fails in
tests/smoke/smoke-all.test.ts(for files annotatedrender-project: true), the failure throws uncaught at module-evaluation time. Deno reports this loudly - exit 1, named error, full stack trace - but everyDeno.test()that would have been registered after the throw is silently never created and never counted. The summary readsN passed | 1 failedwith no indication that M tests never ran at all. This is pre-existing and mode-independent: dev mode and the binary-mode test runner both throw identically for an ordinary render failure, so it is not a regression from binary-mode test support (found while reviewing that PR before merge).Root Cause
render-projectis per-file front matter, not a project-level setting, so a project can mix annotated and unannotated files with the unannotated ones sorting first. A single-passtry/catcharound the pre-render can't isolate the failure: an unannotated sibling file is already registered by the time the annotated file triggers the pre-render, and a failed project would retry its pre-render once per annotated file.Fix
Split the discovery loop into three passes: collect every file first, pre-render each distinct project once, then register tests. A failed project's files are skipped as a group - one named synthetic failing test per project, not a crash of the whole file - while every other file, including ones after it in the file list, keeps running and counting normally.
Cleanup for a skipped file matches a registered one at exact parity: its would-be
projectCleanupEntriesare synthesized, and its own custompostRenderCleanuppaths (registered during discovery regardless of skip status) are swept. That sweep is scoped to the skipped file's own input rather than the whole global list - by the time a skip is handled, every project's pre-render has already run, so an unscoped sweep could delete a different, healthy project's not-yet-verified artifact.Isolating is only safe if the render is known to have stopped.
tests/quarto-cmd.ts's binary-mode process-tree kill previously discarded its outcome - the Windows path losttaskkill's exit code in a local that never escaped, and the Unix path treated any non-throwingpgrepcall as an authoritative enumeration even though a non-zero exit (pgrep's own usage/internal error codes) produces the same empty output as a genuine childless leaf. Cancellation is now a total exit-code partition with a fail-safe default, carried through a newQuartoTimeoutError(renderCancelled,killDetail) instead of a bare string, so dev-mode timeouts (which can never cancel an in-process render) and unconfirmed binary-mode kills both stay fatal rather than being isolated on no evidence.The Unix enumerate-then-kill sequence is still not atomic: a descendant spawned or reparented between the
pgrepwalk and the kill is invisible to it even whencancelledcomes back true. Closing that gap needs a process group/job object, which is out of scope for test infrastructure; the enumeration is the strongest portable signal available without one, and the code documents the limitation rather than implying proof.