fix(runner): record artifacts for flow created browser contexts#1393
fix(runner): record artifacts for flow created browser contexts#1393Chase J (chajac) wants to merge 4 commits into
Conversation
Playwright recording options (recordVideo/recordHar/tracing) are per-context, but createLaunch only instrumented the context it created — contexts a flow made via browser.newContext() recorded nothing (empty HAR, no video, empty trace). Patch browser.newContext in place so every context funnels through one instrumentation path: recording options merged (caller wins), default timeout set, tracing started, and ordered teardown registered. Each context gets indexed HAR/trace paths (flow.har, flow-2.har, ...), which also fixes multiple launch() calls overwriting one another's artifacts. Retain-on-failure cleanup now removes every assigned per-context path. Known gap: a context the flow closes itself flushes video/HAR but loses its trace (tracing must stop before close) — tracked in WIZ-11062.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/runner/createLaunch.ts`:
- Around line 107-110: Update the context creation flow around nextSetup,
originalNewContext, and trackContext to derive the HAR artifact path from the
final merged options, so an opts.recordHar value is tracked instead of the
discarded indexed setup path. Ensure artifactPaths() and cleanup use this
effective caller-provided path, and add a regression test covering a
caller-supplied HAR path.
- Around line 69-83: Update the closeAll teardown flow in createLaunch so it
memoizes and returns the in-progress teardown promise instead of returning
immediately when closed is already true. Ensure concurrent signal-driven
closeAll() and normal cleanup() await the same tracing and close operations
before runWebFlow proceeds to unlink artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58de2e83-7a15-4541-b2be-4cb0a8da9fa0
📒 Files selected for processing (9)
src/domains/runner/createLaunch.contexts.test.tssrc/domains/runner/createLaunch.test.tssrc/domains/runner/createLaunch.tssrc/domains/runner/runWebFlow.har.test.tssrc/domains/runner/runWebFlow.ownContext.fixture.tssrc/domains/runner/runWebFlow.trace.test.tssrc/domains/runner/runWebFlow.tssrc/domains/runner/runWebFlowUtils.tssrc/domains/runner/web/artifactPaths.ts
💤 Files with no reviewable changes (1)
- src/domains/runner/runWebFlowUtils.ts
flows run --video on --har on --trace onsilently records nothing for flows whose helpers create their own browser context. The telltale artifact is a ~218-byte HAR containing"entries": [], no video files, and an empty trace, while the flow itself runs and passes fine.Playwright’s
recordVideo/recordHar/ tracing are per-browser-context options.createLaunchonly instrumented the single context it created insidelaunch(), then returned the rawbrowserhandle. Flow helper code commonly does:Any flow following this pattern (i.e a shared login helper that builds its own context) produced no artifacts locally, with no workaround available via flags.
Overview of Changes
createLaunchnow patchesbrowser.newContextin place, so every context CLI created or flow created funnels through one instrumentation path: recording options merged (caller options win on conflicts), default timeout applied, tracing started, ordered teardown registered.<flow>.har,<flow>-2.harsame for trace zips). This also fixes the preexisting issue where multiplelaunch()calls in one flow overwrote each other’s HAR/trace.retain-on-failurecleanup now removes every assigned per context path, not just the base one.Testing
launch()'s contextbrowser.newContext()entries: []-2.harbun run typecheck bun run lint bun run format:check bun run knip bun run test bun run buildChecklist