Add characterization test for criClient's Chrome launch flags - #14856
Merged
Conversation
Pins the exact Chrome command-line flags criClient builds in src/core/cri/cri.ts before the shared-launcher refactor (PR 14835) lands, so that PR can be verified as behaviour-preserving on the mermaid/graphviz launch path. Stubs the global Deno.Command constructor rather than spawning a real subprocess, so it observes the full call contract (args, stdout, stderr) without depending on which module ends up calling Deno.Command after the refactor.
Global Deno.Command stubbing violates this project's no-mocks testing policy, and coupled the test to stdout/stderr options that the pending shared-launcher refactor changes from "piped" to "null". Spawning a generated fake executable observes the same argv contract criClient's callers rely on without depending on internal call structure, so it survives the refactor unedited. Drops the "Chrome never becomes ready" case; that belongs with the refactor's own launchChrome() timeout handling rather than this flag-preservation test.
… dir A fixed setTimeout self-exit left the shutdown timing to guesswork and never cleaned the temp directory. Hitting a dedicated /shutdown route after criClient resolves terminates the process deterministically, and the temp dir removal now runs in a finally so it happens even if an assertion throws.
An assertion failure before the shutdown request left the fake Chrome process running indefinitely. Requesting shutdown now happens in the finally block, and the temp dir is only removed once the port stops answering -- confirming the process has actually exited -- rather than racing its exit timer, which could otherwise fail to remove a script file the process still has open.
…lowing it waitForPortClosed's timeout and the temp-dir removal were both silently ignored, so a process that failed to shut down would leak with no visible signal in the test output. Report both cases via console.error without throwing from the finally block, so a genuine cleanup failure becomes visible without masking whatever assertion error is already propagating.
Logging a stuck process or a failed directory removal let the test pass while still leaking state. Cleanup errors are now collected and thrown via AggregateError, combined with any assertion failure that already occurred rather than overwriting it -- a plain second throw from a finally block would have silently discarded the original error.
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. |
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.
PR #14835 moves criClient's Chrome argument-building into a shared launcher. This pins criClient's current command-line flags first, so that refactor can be verified as behavior-preserving rather than trusted by inspection.
The test spawns a generated fake Chrome executable instead of mocking
Deno.Command, so it observes the same argv contract any real Chrome launch depends on, independent of which module ends up building it -- surviving the refactor without needing to change.