diff --git a/.changeset/gitignored-default-output-dir.md b/.changeset/gitignored-default-output-dir.md new file mode 100644 index 000000000..b6f607b2f --- /dev/null +++ b/.changeset/gitignored-default-output-dir.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Change the default `flows run` output directory from `qawolf-output` to `.qawolf/output`, so run artifacts (videos, traces, HAR) land under the `.qawolf/` directory that `qawolf init` gitignores. Pass `--output-dir` to override. diff --git a/.changeset/har-trace-artifacts.md b/.changeset/har-trace-artifacts.md new file mode 100644 index 000000000..4ee6c7930 --- /dev/null +++ b/.changeset/har-trace-artifacts.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Save HAR and trace artifacts reliably from `flows run`. HAR (and video) could be silently dropped because contexts and browsers were closed concurrently, racing Playwright's artifact flush; contexts now close first. The `--trace` flag is now wired end-to-end and writes a Playwright trace to `/trace/.zip`, honoring `on`, `off`, and `retain-on-failure`. diff --git a/.changeset/timeout-web-flow-expect.md b/.changeset/timeout-web-flow-expect.md new file mode 100644 index 000000000..09db3a44b --- /dev/null +++ b/.changeset/timeout-web-flow-expect.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +Wire `flows run --timeout` into the web flow expect timeout. Previously the flag only set Playwright's per-action timeout, so assertion failures still waited the hardcoded 30s default; it now applies to actions and assertions alike. diff --git a/src/commands/__snapshots__/help.test.ts.snap b/src/commands/__snapshots__/help.test.ts.snap index 982ee71ed..1726eb3a2 100644 --- a/src/commands/__snapshots__/help.test.ts.snap +++ b/src/commands/__snapshots__/help.test.ts.snap @@ -137,7 +137,8 @@ Options: 0) --bail Stop the run after the first failure (default: false) --workers Parallel worker count for web flows (default: 1) - --timeout Per-flow timeout in milliseconds (default: 30000) + --timeout Default timeout for actions and assertions, in + milliseconds (default: 30000) --video Record video: on | off | retain-on-failure (default: "off") --trace Record Playwright trace: on | off | retain-on-failure @@ -147,7 +148,7 @@ Options: --har-content HAR response bodies: omit | full (full uses more memory) (default: "omit") --output-dir Directory for run artifacts (videos, traces, HAR) - (default: "qawolf-output") + (default: ".qawolf/output") --junit [path] Write a JUnit XML report (default: /junit-report.xml) --headed Show the browser window instead of headless (default: diff --git a/src/commands/flows/run.register.ts b/src/commands/flows/run.register.ts index b8ef19675..462c94510 100644 --- a/src/commands/flows/run.register.ts +++ b/src/commands/flows/run.register.ts @@ -9,6 +9,7 @@ import type { TraceMode, VideoMode, } from "~/core/types.js"; +import { defaultOutputDir } from "~/core/paths.js"; import { parseEnum, parseInteger } from "~/domains/runner/runFlagParsers.js"; import type { FlowsRunFlags } from "~/domains/runner/runInternals.js"; @@ -56,7 +57,7 @@ export function registerFlowsRunCommand( ) .option( "--timeout ", - "Per-flow timeout in milliseconds", + "Default timeout for actions and assertions, in milliseconds", parseInteger("--timeout", { min: 0 }), 30_000, ) @@ -87,7 +88,7 @@ export function registerFlowsRunCommand( .option( "--output-dir ", "Directory for run artifacts (videos, traces, HAR)", - "qawolf-output", + defaultOutputDir, ) .option( "--junit [path]", diff --git a/src/core/messages/runner.ts b/src/core/messages/runner.ts index ac292a7f3..875f3f00e 100644 --- a/src/core/messages/runner.ts +++ b/src/core/messages/runner.ts @@ -16,6 +16,10 @@ export const runnerMessages = { `retries must be a non-negative integer, got ${String(retries)}`, manifestStampReadFailed: (file: string, message: string) => `failed to read manifest stamp for ${file}: ${message}`, + harCleanupFailed: (file: string, message: string) => + `failed to delete HAR file ${file} (retain-on-failure); remove it manually: ${message}`, + traceCleanupFailed: (file: string, message: string) => + `failed to delete trace file ${file} (retain-on-failure); remove it manually: ${message}`, notSupportedInCli: (name: string) => `${name} is not supported in the CLI runner`, notAvailableLocally: (name: string) => diff --git a/src/core/paths.test.ts b/src/core/paths.test.ts new file mode 100644 index 000000000..7fd18ed33 --- /dev/null +++ b/src/core/paths.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, it } from "bun:test"; + +import { defaultOutputDir, qawolfDir } from "./paths.js"; + +describe("defaultOutputDir", () => { + it("is nested inside the qawolf directory that init gitignores", () => { + expect(defaultOutputDir.startsWith(`${qawolfDir}/`)).toBe(true); + }); + + it("is a relative path so artifacts land in the project, not an absolute location", () => { + expect(defaultOutputDir.startsWith("/")).toBe(false); + }); +}); diff --git a/src/core/paths.ts b/src/core/paths.ts index bc07e987d..2eb0d697f 100644 --- a/src/core/paths.ts +++ b/src/core/paths.ts @@ -1,5 +1,18 @@ import envPaths from "env-paths"; +/** + * Directory `qawolf init` writes a catch-all `.gitignore` into, so anything + * nested under it is excluded from version control by default. + */ +export const qawolfDir = ".qawolf"; + +/** + * Default directory for `flows run` artifacts (videos, traces, HAR). Nested + * under {@link qawolfDir} so the `.gitignore` written by `qawolf init` keeps + * run artifacts out of version control. + */ +export const defaultOutputDir = `${qawolfDir}/output`; + let _paths: ReturnType | undefined; export function getConfigDir(): string { diff --git a/src/domains/config/loadConfig.test.ts b/src/domains/config/loadConfig.test.ts index 4cb6217dd..555bf81d5 100644 --- a/src/domains/config/loadConfig.test.ts +++ b/src/domains/config/loadConfig.test.ts @@ -30,7 +30,7 @@ describe("loadConfig", () => { it("returns defaults when qawolf.config.ts is missing", async () => { const config = await loadConfig(missingConfig()); expect(config).toEqual({ - outputDir: ".qawolf", + outputDir: ".qawolf/output", timeout: 60_000, retries: 0, bail: false, diff --git a/src/domains/config/schema.ts b/src/domains/config/schema.ts index 549139c8e..c9d20ab0c 100644 --- a/src/domains/config/schema.ts +++ b/src/domains/config/schema.ts @@ -1,9 +1,11 @@ import { z } from "zod"; +import { defaultOutputDir } from "~/core/paths.js"; + const artifactModeSchema = z.enum(["on", "off", "retain-on-failure"]); export const qawolfConfigSchema = z.strictObject({ - outputDir: z.string().default(".qawolf"), + outputDir: z.string().default(defaultOutputDir), timeout: z.number().int().positive().default(60_000), retries: z.number().int().min(0).default(0), bail: z.boolean().default(false), diff --git a/src/domains/init/templates.ts b/src/domains/init/templates.ts index 78cf6c342..90f919a3d 100644 --- a/src/domains/init/templates.ts +++ b/src/domains/init/templates.ts @@ -1,5 +1,7 @@ +import { defaultOutputDir } from "~/core/paths.js"; + export const qawolfConfigTs = `export default { - outputDir: ".qawolf", + outputDir: "${defaultOutputDir}", timeout: 60_000, retries: 0, video: "retain-on-failure", diff --git a/src/domains/runner/dispatchViaSubprocess.test.ts b/src/domains/runner/dispatchViaSubprocess.test.ts index 13eef15a5..87f83678a 100644 --- a/src/domains/runner/dispatchViaSubprocess.test.ts +++ b/src/domains/runner/dispatchViaSubprocess.test.ts @@ -19,6 +19,7 @@ const webOptions: RunWebFlowOptions = { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, har: "off", harContent: "omit", diff --git a/src/domains/runner/executeWorkerFlow.test.ts b/src/domains/runner/executeWorkerFlow.test.ts index b1bac616b..e5179b24d 100644 --- a/src/domains/runner/executeWorkerFlow.test.ts +++ b/src/domains/runner/executeWorkerFlow.test.ts @@ -25,6 +25,7 @@ function makeInput(): WorkerInput { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, har: "off", harContent: "omit", diff --git a/src/domains/runner/initFlowRuntime.test.ts b/src/domains/runner/initFlowRuntime.test.ts index cb1ab3198..74bb3b3e1 100644 --- a/src/domains/runner/initFlowRuntime.test.ts +++ b/src/domains/runner/initFlowRuntime.test.ts @@ -10,20 +10,47 @@ afterEach(() => { const thisDir = import.meta.dirname; +/** Reads back the expect timeout the runner configured on @qawolf/flows. */ +async function readConfiguredExpectTimeout(): Promise { + const idxUrl = import.meta.resolve("@qawolf/flows"); + const attrsUrl = new URL("./web/expect/attributes.js", idxUrl).href; + const { getWebExpectAttributes } = (await import(attrsUrl)) as { + getWebExpectAttributes: () => { defaultExpectTimeoutMs: number }; + }; + return getWebExpectAttributes().defaultExpectTimeoutMs; +} + describe("initFlowRuntime", () => { it("resolves using the CLI's own @qawolf/flows", async () => { - await initFlowRuntime(path.join(thisDir, "fake.flow.ts")); + await initFlowRuntime(path.join(thisDir, "fake.flow.ts"), { + timeout: 30_000, + }); + }); + + it("configures the @qawolf/flows expect timeout from the passed timeout", async () => { + await initFlowRuntime(path.join(thisDir, "fake.flow.ts"), { + timeout: 5_000, + }); + expect(await readConfiguredExpectTimeout()).toBe(5_000); }); it("returns the same promise for repeated calls from the same directory", () => { - const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts")); - const p2 = initFlowRuntime(path.join(thisDir, "b.flow.ts")); + const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts"), { + timeout: 30_000, + }); + const p2 = initFlowRuntime(path.join(thisDir, "b.flow.ts"), { + timeout: 30_000, + }); expect(p1).toBe(p2); }); it("returns a different promise for a different starting directory", () => { - const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts")); - const p2 = initFlowRuntime(path.join(thisDir, "sub", "b.flow.ts")); + const p1 = initFlowRuntime(path.join(thisDir, "a.flow.ts"), { + timeout: 30_000, + }); + const p2 = initFlowRuntime(path.join(thisDir, "sub", "b.flow.ts"), { + timeout: 30_000, + }); expect(p1).not.toBe(p2); // settle both so they don't leak into subsequent tests return Promise.allSettled([p1, p2]); @@ -34,7 +61,9 @@ describe("initFlowRuntime", () => { try { let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } @@ -60,7 +89,9 @@ describe("initFlowRuntime", () => { ); let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } @@ -82,7 +113,9 @@ describe("initFlowRuntime", () => { await mkdir(path.join(pkgDir, "package.json")); let caught: unknown; try { - await initFlowRuntime(path.join(tmp, "my.flow.ts")); + await initFlowRuntime(path.join(tmp, "my.flow.ts"), { + timeout: 30_000, + }); } catch (e) { caught = e; } diff --git a/src/domains/runner/initFlowRuntime.ts b/src/domains/runner/initFlowRuntime.ts index e433005e2..b5af90a06 100644 --- a/src/domains/runner/initFlowRuntime.ts +++ b/src/domains/runner/initFlowRuntime.ts @@ -6,9 +6,20 @@ import { isNoEntError } from "~/core/errors.js"; type ConfigureFlowRuntime = (opts: { target: unknown; - webExpectAttributes?: unknown; + webExpectAttributes?: { defaultExpectTimeoutMs: number }; }) => Promise; +export type InitFlowRuntimeOptions = { + /** + * Default timeout (ms) for flow actions and assertions. Threaded into + * @qawolf/flows as `defaultExpectTimeoutMs` so the package's `expect` + * wrapper honors `--timeout`; without it the wrapper pins every assertion + * to its hardcoded 30s default. The matching Playwright action timeout is + * applied separately via `context.setDefaultTimeout` at launch. + */ + timeout: number; +}; + async function findFlowsRunnerPath(flowPath: string, fs: Fs): Promise { let dir = path.dirname(flowPath); while (true) { @@ -47,7 +58,11 @@ async function findFlowsRunnerPath(flowPath: string, fs: Fs): Promise { const initCache = new Map>(); -async function doInit(flowPath: string, fs: Fs): Promise { +async function doInit( + flowPath: string, + timeout: number, + fs: Fs, +): Promise { const runnerPath = await findFlowsRunnerPath(flowPath, fs); const mod = (await import(pathToFileURL(runnerPath).href)) as { configureFlowRuntime?: ConfigureFlowRuntime; @@ -63,18 +78,22 @@ async function doInit(flowPath: string, fs: Fs): Promise { runnerName: "node20WithPlaywright", meta: "legacy", }, + webExpectAttributes: { defaultExpectTimeoutMs: timeout }, }); } export function initFlowRuntime( flowPath: string, + options: InitFlowRuntimeOptions, fs: Fs = makeDefaultFs(), ): Promise { const startDir = path.dirname(flowPath); - // Cache key is startDir, not fs — tests reusing the same startDir must call _resetInitCache() between runs. + // Cache key is startDir, not fs — tests reusing the same startDir must call + // _resetInitCache() between runs. Timeout is omitted deliberately: it is a + // single run-global flag, so every flow in a process shares one value. let p = initCache.get(startDir); if (!p) { - p = doInit(flowPath, fs); + p = doInit(flowPath, options.timeout, fs); initCache.set(startDir, p); } return p; diff --git a/src/domains/runner/run.manifestStamp.test.ts b/src/domains/runner/run.manifestStamp.test.ts index e9b84cecb..e9dd697d6 100644 --- a/src/domains/runner/run.manifestStamp.test.ts +++ b/src/domains/runner/run.manifestStamp.test.ts @@ -62,6 +62,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { @@ -104,6 +105,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { @@ -142,6 +144,7 @@ describe("dispatchFlow manifest stamping", () => { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: defaultFlags().timeout, }, androidOptions: { diff --git a/src/domains/runner/runHelpers.test.ts b/src/domains/runner/runHelpers.test.ts index 81c337f58..2c46e1817 100644 --- a/src/domains/runner/runHelpers.test.ts +++ b/src/domains/runner/runHelpers.test.ts @@ -16,4 +16,9 @@ describe("buildRunOptions", () => { const { webOptions } = buildRunOptions({ ...defaultFlags(), headed: true }); expect(webOptions.headed).toBe(true); }); + + it("passes the trace mode through to webOptions", () => { + const { webOptions } = buildRunOptions({ ...defaultFlags(), trace: "on" }); + expect(webOptions.trace).toBe("on"); + }); }); diff --git a/src/domains/runner/runHelpers.ts b/src/domains/runner/runHelpers.ts index 67d1fe342..feac509cb 100644 --- a/src/domains/runner/runHelpers.ts +++ b/src/domains/runner/runHelpers.ts @@ -31,6 +31,7 @@ export function buildRunOptions(flags: FlowsRunFlags): { headed: flags.headed, slowMo: 0, video: flags.video, + trace: flags.trace, timeout: flags.timeout, har: flags.har, harContent: flags.harContent, diff --git a/src/domains/runner/runWebFlow.fixtures.ts b/src/domains/runner/runWebFlow.fixtures.ts index ca750d401..a7468b358 100644 --- a/src/domains/runner/runWebFlow.fixtures.ts +++ b/src/domains/runner/runWebFlow.fixtures.ts @@ -43,6 +43,7 @@ export const baseOptions: RunWebFlowOptions = { headed: false, slowMo: 0, video: "off", + trace: "off", timeout: 30_000, }; diff --git a/src/domains/runner/runWebFlow.test.ts b/src/domains/runner/runWebFlow.test.ts index c01d367a1..fe56de1c4 100644 --- a/src/domains/runner/runWebFlow.test.ts +++ b/src/domains/runner/runWebFlow.test.ts @@ -7,6 +7,7 @@ import { makeUniformDeps, } from "./web/createWebLaunchContext.fixtures.js"; import { runWebFlow } from "./runWebFlow.js"; +import { _resetInitCache } from "./initFlowRuntime.js"; import { baseOptions, fixturePath, @@ -14,10 +15,38 @@ import { } from "./runWebFlow.fixtures.js"; afterEach(() => { + // The expect-timeout readback test configures a process-global on + // @qawolf/flows; clear the init cache so it does not leak into later tests. + _resetInitCache(); mock.restore(); }); +/** Reads back the expect timeout configured on @qawolf/flows. */ +async function readConfiguredExpectTimeout(): Promise { + const idxUrl = import.meta.resolve("@qawolf/flows"); + const attrsUrl = new URL("./web/expect/attributes.js", idxUrl).href; + const { getWebExpectAttributes } = (await import(attrsUrl)) as { + getWebExpectAttributes: () => { defaultExpectTimeoutMs: number }; + }; + return getWebExpectAttributes().defaultExpectTimeoutMs; +} + describe("runWebFlow", () => { + it("should configure the @qawolf/flows expect timeout from options.timeout", async () => { + // initFlowRuntime memoizes per flow directory and configures a + // process-global on @qawolf/flows. Other runner tests share this + // directory, so clear the cache to force a fresh configure here. + _resetInitCache(); + + await runWebFlow({ + deps: makeWebDeps(), + options: { ...baseOptions, timeout: 7_777 }, + flowPath: fixturePath("pass"), + }); + + expect(await readConfiguredExpectTimeout()).toBe(7_777); + }); + it("should return passed: true when the flow succeeds", async () => { const result = await runWebFlow({ deps: makeWebDeps(), diff --git a/src/domains/runner/runWebFlow.trace.test.ts b/src/domains/runner/runWebFlow.trace.test.ts new file mode 100644 index 000000000..8ce5d765f --- /dev/null +++ b/src/domains/runner/runWebFlow.trace.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { + makeBrowser, + makeContext, + makeDep, + makePage, + makeUniformDeps, +} from "./web/createWebLaunchContext.fixtures.js"; +import { runWebFlow } from "./runWebFlow.js"; +import { + baseOptions, + fixturePath, + makeWebDeps, +} from "./runWebFlow.fixtures.js"; + +afterEach(() => { + mock.restore(); +}); + +describe("runWebFlow trace", () => { + it("should not start tracing when trace is off", async () => { + const ctx = makeContext([makePage()]); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const deps = makeWebDeps(makeUniformDeps(makeDep(makeBrowser(ctx), ctx))); + + await runWebFlow({ + deps, + options: baseOptions, + flowPath: fixturePath("launch"), + }); + + expect(startMock).not.toHaveBeenCalled(); + }); + + it("should start and stop tracing to the trace path when trace is on", async () => { + const ctx = makeContext([makePage()]); + const startMock = mock(async () => {}); + const stopMock = mock(async (_opts: { path?: string }) => {}); + ctx.tracing = { start: startMock, stop: stopMock }; + const deps = makeWebDeps(makeUniformDeps(makeDep(makeBrowser(ctx), ctx))); + + await runWebFlow({ + deps, + options: { ...baseOptions, trace: "on", outputDir: "/tmp/test-trace" }, + flowPath: fixturePath("launch"), + }); + + expect(startMock).toHaveBeenCalledTimes(1); + const stopArg = stopMock.mock.calls[0]![0]; + expect(stopArg.path).toContain("/trace/launch.zip"); + }); + + it("should delete the trace file on success when trace is retain-on-failure", async () => { + const unlinkMock = mock(async (_p: string) => {}); + const deps = { + ...makeWebDeps(), + fs: { + mkdir: async () => {}, + writeFile: async () => {}, + unlink: unlinkMock, + }, + }; + + await runWebFlow({ + deps, + options: { + ...baseOptions, + trace: "retain-on-failure", + outputDir: "/tmp/test-trace", + }, + flowPath: fixturePath("pass"), + }); + + expect(unlinkMock.mock.calls[0]![0]).toContain("/trace/"); + }); + + it("should not delete the trace file on failure when trace is retain-on-failure", async () => { + const unlinkMock = mock(async (_p: string) => {}); + const deps = { + ...makeWebDeps(), + fs: { + mkdir: async () => {}, + writeFile: async () => {}, + unlink: unlinkMock, + }, + }; + + await runWebFlow({ + deps, + options: { + ...baseOptions, + trace: "retain-on-failure", + outputDir: "/tmp/test-trace", + }, + flowPath: fixturePath("fail"), + }); + + expect(unlinkMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/domains/runner/runWebFlow.ts b/src/domains/runner/runWebFlow.ts index d04f68d60..6c851a06a 100644 --- a/src/domains/runner/runWebFlow.ts +++ b/src/domains/runner/runWebFlow.ts @@ -23,13 +23,14 @@ import { import { buildContextSetup, initHar, + initTrace, maybeCleanupHar, + maybeCleanupTrace, } from "./web/contextSetup.js"; export type RunWebFlowDeps = RunnerDeps & WebLaunchDeps; -// trace is not yet implemented export type RunWebFlowOptions = RunnerOptions & - Omit; + Omit; export async function runWebFlow({ deps, @@ -40,7 +41,7 @@ export async function runWebFlow({ options: RunWebFlowOptions; flowPath: string; }): Promise { - await initFlowRuntime(flowPath); + await initFlowRuntime(flowPath, { timeout: options.timeout }); const exported = await loadFlowDefault(flowPath); @@ -53,6 +54,7 @@ export async function runWebFlow({ : (exported as WebFlowDefinition).run; const { harMode, harPath } = await initHar(deps.fs, options, flowName); + const { traceMode, tracePath } = await initTrace(deps.fs, options, flowName); const videoSize = { width: 1280, height: 720 }; const contextSetup = buildContextSetup(videoSize, options, harPath); @@ -74,6 +76,8 @@ export async function runWebFlow({ launchBrowserOpts, signals: deps.signals, timeout: options.timeout, + traceMode, + tracePath, }); const flowDef: FlowDefinition = { @@ -106,8 +110,12 @@ export async function runWebFlow({ return result; } finally { await cleanup(); + const warn = (message: string) => deps.logger?.warn(message); if (harPath !== undefined) { - await maybeCleanupHar(deps.fs, harPath, passed, harMode); + await maybeCleanupHar(deps.fs, harPath, passed, harMode, warn); + } + if (tracePath !== undefined) { + await maybeCleanupTrace(deps.fs, tracePath, passed, traceMode, warn); } } } diff --git a/src/domains/runner/runWebFlowUtils.test.ts b/src/domains/runner/runWebFlowUtils.test.ts new file mode 100644 index 000000000..f119b33fc --- /dev/null +++ b/src/domains/runner/runWebFlowUtils.test.ts @@ -0,0 +1,147 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { createLaunch } from "./runWebFlowUtils.js"; +import { + makeBrowser, + makeContext, + makeDep, +} from "./web/createWebLaunchContext.fixtures.js"; +import { makeNoopSignals } from "~/shell/signals/createSignalRegistry.fixtures.js"; +import { createSignalRegistry } from "~/shell/signals/createSignalRegistry.js"; + +afterEach(() => { + mock.restore(); +}); + +const launchBrowserOpts = { headless: true, slowMo: 0 }; + +describe("createLaunch cleanup", () => { + it("should close browsers only after contexts have fully closed", async () => { + const order: string[] = []; + const ctx = makeContext(); + ctx.close = mock(async () => { + order.push("context:start"); + await Promise.resolve(); + order.push("context:end"); + }); + const browser = makeBrowser(ctx); + browser.close = mock(async () => { + order.push("browser:start"); + }); + const dep = makeDep(browser, ctx); + + const { launch, cleanup } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + }); + + await launch(); + await cleanup(); + + expect(order).toEqual(["context:start", "context:end", "browser:start"]); + }); + + it("should close contexts before browsers on signal-driven shutdown", async () => { + const order: string[] = []; + const ctx = makeContext(); + ctx.close = mock(async () => { + order.push("context:start"); + await Promise.resolve(); + order.push("context:end"); + }); + const browser = makeBrowser(ctx); + browser.close = mock(async () => { + order.push("browser:start"); + }); + const dep = makeDep(browser, ctx); + const signals = createSignalRegistry(); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals, + timeout: 30_000, + }); + + await launch(); + await signals.shutdown("SIGINT"); + + expect(order).toEqual(["context:start", "context:end", "browser:start"]); + }); +}); + +describe("createLaunch tracing", () => { + it("should start tracing after creating the context when trace is on", async () => { + const ctx = makeContext(); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "on", + tracePath: "/out/trace/flow.zip", + }); + + await launch(); + + expect(startMock).toHaveBeenCalledTimes(1); + }); + + it("should not start tracing when trace is off", async () => { + const ctx = makeContext(); + const startMock = mock(async () => {}); + ctx.tracing = { start: startMock, stop: async () => {} }; + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "off", + tracePath: undefined, + }); + + await launch(); + + expect(startMock).not.toHaveBeenCalled(); + }); + + it("should stop tracing to the trace path before closing the context", async () => { + const order: string[] = []; + const ctx = makeContext(); + const stopMock = mock(async (_opts: { path?: string }) => { + order.push("trace:stop"); + }); + ctx.tracing = { start: async () => {}, stop: stopMock }; + ctx.close = mock(async () => { + order.push("context:close"); + }); + const dep = makeDep(makeBrowser(ctx), ctx); + + const { launch, cleanup } = createLaunch({ + browsers: { chromium: dep, firefox: dep, webkit: dep }, + contextSetup: {}, + launchBrowserOpts, + signals: makeNoopSignals(), + timeout: 30_000, + traceMode: "on", + tracePath: "/out/trace/flow.zip", + }); + + await launch(); + await cleanup(); + + expect(stopMock).toHaveBeenCalledWith({ path: "/out/trace/flow.zip" }); + expect(order).toEqual(["trace:stop", "context:close"]); + }); +}); diff --git a/src/domains/runner/runWebFlowUtils.ts b/src/domains/runner/runWebFlowUtils.ts index c13713035..07424b809 100644 --- a/src/domains/runner/runWebFlowUtils.ts +++ b/src/domains/runner/runWebFlowUtils.ts @@ -1,6 +1,7 @@ import crypto from "node:crypto"; import os from "node:os"; import path from "node:path"; +import type { TraceMode } from "~/core/types.js"; import type { SignalRegistry } from "./types.js"; import { runnerMessages } from "~/core/messages/index.js"; import { unsupportedSharedDepNames } from "./unsupportedDepNames.js"; @@ -32,20 +33,51 @@ export function createLaunch({ launchBrowserOpts, signals, timeout, + traceMode = "off", + tracePath, }: { browsers: { chromium: BrowserDep; firefox: BrowserDep; webkit: BrowserDep }; contextSetup: ContextSetupOptions; launchBrowserOpts: BrowserLaunchOptions; signals: SignalRegistry; timeout: number; + traceMode?: TraceMode; + tracePath?: string | undefined; }): { launch: LaunchFn; cleanup: () => Promise } { const openBrowsers: MinimalBrowser[] = []; const openContexts: MinimalBrowserContext[] = []; - const unregisters: (() => void)[] = []; + const tracingEnabled = traceMode !== "off"; + let closed = false; - const trackContext = (ctx: MinimalBrowserContext) => { + // Ordered teardown shared by normal cleanup and signal-driven shutdown: + // stop tracing, then close contexts, then close browsers. The order matters + // because Playwright flushes HAR/video during context.close() and writes the + // trace on tracing.stop(); a browser.close() racing either can terminate the + // connection mid-flush and silently drop the artifact. + // TODO WIZ-10839: a flow with multiple launch() calls stops every context to + // the same tracePath, so only one trace zip survives (mirrors HAR). + const closeAll = async () => { + if (closed) return; + closed = true; + if (tracingEnabled && tracePath !== undefined) { + await Promise.allSettled( + openContexts.map((c) => c.tracing.stop({ path: tracePath })), + ); + } + await Promise.allSettled(openContexts.map((c) => c.close())); + await Promise.allSettled(openBrowsers.map((b) => b.close())); + }; + + // Register the ordered teardown once. SignalRegistry runs cleanups + // concurrently, so registering context and browser closes separately would + // let browser.close() race the context flush on a SIGINT-interrupted run. + const unregister = signals.register(closeAll); + + const trackContext = async (ctx: MinimalBrowserContext) => { openContexts.push(ctx); - unregisters.push(signals.register(() => ctx.close())); + if (tracingEnabled) { + await ctx.tracing.start({ screenshots: true, snapshots: true }); + } }; const launch: LaunchFn = async (launchOpts) => { @@ -61,26 +93,22 @@ export function createLaunch({ ...contextSetup, }); context.setDefaultTimeout(timeout); - trackContext(context); + await trackContext(context); const page = await context.newPage(); return { browserType: browserName, context, page }; } const browser = await bt.launch(launchBrowserOpts); openBrowsers.push(browser); - unregisters.push(signals.register(() => browser.close())); const context = await browser.newContext(contextSetup); context.setDefaultTimeout(timeout); - trackContext(context); + await trackContext(context); return { browser, browserType: browserName, context }; }; const cleanup = async () => { - for (const unreg of unregisters) unreg(); - await Promise.allSettled([ - ...openContexts.map((c) => c.close()), - ...openBrowsers.map((b) => b.close()), - ]); + unregister(); + await closeAll(); }; return { launch, cleanup }; diff --git a/src/domains/runner/web/contextSetup.test.ts b/src/domains/runner/web/contextSetup.test.ts index b57a22119..f5c2cac86 100644 --- a/src/domains/runner/web/contextSetup.test.ts +++ b/src/domains/runner/web/contextSetup.test.ts @@ -82,6 +82,24 @@ describe("maybeCleanupHar", () => { ); expect(result).resolves.toBeUndefined(); }); + + it("should warn with the HAR path when unlink rejects", async () => { + const warn = mock((_msg: string) => {}); + const fs = { + unlink: mock(async () => { + throw new Error("EPERM"); + }), + }; + await maybeCleanupHar( + fs, + "/out/har/flow.har", + true, + "retain-on-failure", + warn, + ); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toContain("/out/har/flow.har"); + }); }); describe("initHar", () => { diff --git a/src/domains/runner/web/contextSetup.trace.test.ts b/src/domains/runner/web/contextSetup.trace.test.ts new file mode 100644 index 000000000..c7a856eb1 --- /dev/null +++ b/src/domains/runner/web/contextSetup.trace.test.ts @@ -0,0 +1,115 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { initTrace, maybeCleanupTrace, traceFlowPath } from "./contextSetup.js"; + +afterEach(() => { + mock.restore(); +}); + +describe("traceFlowPath", () => { + it("should return /trace/.zip", () => { + expect(traceFlowPath("/out", "my-flow")).toBe("/out/trace/my-flow.zip"); + }); +}); + +describe("initTrace", () => { + it("should return traceMode off and tracePath undefined when trace is off", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace(fs, { outputDir: "/out" }, "flow"); + expect(result.traceMode).toBe("off"); + expect(result.tracePath).toBeUndefined(); + expect(mkdirMock).not.toHaveBeenCalled(); + }); + + it("should return traceMode and tracePath and call mkdir when trace is on", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace( + fs, + { trace: "on", outputDir: "/out" }, + "my-flow", + ); + expect(result.traceMode).toBe("on"); + expect(result.tracePath).toBe("/out/trace/my-flow.zip"); + expect(mkdirMock).toHaveBeenCalledWith("/out/trace", { recursive: true }); + }); + + it("should return traceMode and tracePath and call mkdir when trace is retain-on-failure", async () => { + const mkdirMock = mock(async () => {}); + const fs = { mkdir: mkdirMock }; + const result = await initTrace( + fs, + { trace: "retain-on-failure", outputDir: "/out" }, + "my-flow", + ); + expect(result.traceMode).toBe("retain-on-failure"); + expect(result.tracePath).toBe("/out/trace/my-flow.zip"); + expect(mkdirMock).toHaveBeenCalledWith("/out/trace", { recursive: true }); + }); +}); + +describe("maybeCleanupTrace", () => { + it("should call unlink when traceMode is retain-on-failure and flow passed", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + ); + expect(unlinkMock).toHaveBeenCalledWith("/out/trace/flow.zip"); + }); + + it("should not call unlink when traceMode is retain-on-failure and flow failed", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + false, + "retain-on-failure", + ); + expect(unlinkMock).not.toHaveBeenCalled(); + }); + + it("should not call unlink when traceMode is on", async () => { + const unlinkMock = mock(async () => {}); + const fs = { unlink: unlinkMock }; + await maybeCleanupTrace(fs, "/out/trace/flow.zip", true, "on"); + expect(unlinkMock).not.toHaveBeenCalled(); + }); + + it("should not throw when unlink rejects", async () => { + const fs = { + unlink: mock(async () => { + throw new Error("ENOENT"); + }), + }; + const result = maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + ); + expect(result).resolves.toBeUndefined(); + }); + + it("should warn with the trace path when unlink rejects", async () => { + const warn = mock((_msg: string) => {}); + const fs = { + unlink: mock(async () => { + throw new Error("EPERM"); + }), + }; + await maybeCleanupTrace( + fs, + "/out/trace/flow.zip", + true, + "retain-on-failure", + warn, + ); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toContain("/out/trace/flow.zip"); + }); +}); diff --git a/src/domains/runner/web/contextSetup.ts b/src/domains/runner/web/contextSetup.ts index 5c438995d..2d33c3ac5 100644 --- a/src/domains/runner/web/contextSetup.ts +++ b/src/domains/runner/web/contextSetup.ts @@ -1,5 +1,12 @@ import path from "node:path"; -import type { HarContent, HarMode, VideoMode } from "~/core/types.js"; +import { errorMessage } from "~/core/errors.js"; +import { runnerMessages } from "~/core/messages/index.js"; +import type { + HarContent, + HarMode, + TraceMode, + VideoMode, +} from "~/core/types.js"; import type { ContextSetupOptions } from "./types.js"; /** Returns the path where Playwright writes the HAR file for a single flow run. */ @@ -7,6 +14,11 @@ export function harFlowPath(outputDir: string, flowName: string): string { return path.join(outputDir, "har", `${flowName}.har`); } +/** Returns the path where Playwright writes the trace zip for a single flow run. */ +export function traceFlowPath(outputDir: string, flowName: string): string { + return path.join(outputDir, "trace", `${flowName}.zip`); +} + /** * Returns the Playwright `recordHar` context option. * Always sets `mode: "minimal"` (captures URL, status, headers, timing; skips @@ -46,6 +58,40 @@ export async function initHar( return { harMode, harPath }; } +/** Resolves traceMode and tracePath, creating the output directory when needed. */ +export async function initTrace( + fs: { mkdir: (p: string, opts?: { recursive?: boolean }) => Promise }, + options: { trace?: TraceMode; outputDir: string }, + flowName: string, +): Promise<{ traceMode: TraceMode; tracePath: string | undefined }> { + const traceMode = options.trace ?? "off"; + if (traceMode === "off") return { traceMode, tracePath: undefined }; + const tracePath = traceFlowPath(options.outputDir, flowName); + await fs.mkdir(path.dirname(tracePath), { recursive: true }); + return { traceMode, tracePath }; +} + +/** + * Deletes the trace file when `traceMode` is `"retain-on-failure"` and the flow + * passed. No-ops for `"on"` and `"off"` modes. + * A failed `unlink` must not fail a passing flow, so the error is swallowed — + * but `warn` is called with the path so the user can remove the file manually. + */ +export async function maybeCleanupTrace( + fs: { unlink: (p: string) => Promise }, + tracePath: string, + passed: boolean, + traceMode: TraceMode, + warn?: (message: string) => void, +): Promise { + if (traceMode !== "retain-on-failure" || !passed) return; + try { + await fs.unlink(tracePath); + } catch (err) { + warn?.(runnerMessages.traceCleanupFailed(tracePath, errorMessage(err))); + } +} + /** Builds the Playwright context setup options for video and HAR recording. */ export function buildContextSetup( videoSize: { width: number; height: number }, @@ -76,18 +122,20 @@ export function buildContextSetup( /** * Deletes the HAR file when `harMode` is `"retain-on-failure"` and the flow * passed. No-ops for `"on"` and `"off"` modes. - * Errors from `unlink` are swallowed — cleanup failure must not fail a passing flow. + * A failed `unlink` must not fail a passing flow, so the error is swallowed — + * but `warn` is called with the path so the user can remove the file manually. */ export async function maybeCleanupHar( fs: { unlink: (p: string) => Promise }, harPath: string, passed: boolean, harMode: HarMode, + warn?: (message: string) => void, ): Promise { if (harMode !== "retain-on-failure" || !passed) return; try { await fs.unlink(harPath); - } catch { - // best-effort; do not surface cleanup errors + } catch (err) { + warn?.(runnerMessages.harCleanupFailed(harPath, errorMessage(err))); } }