Skip to content

feat: add bail-on-failure, granular recording, env var interpolation,secret masking, and trace viewer control - #15

Merged
elonsalfati merged 8 commits into
salfatigroup:mainfrom
techjacker:feat/testing-improvements
Apr 1, 2026
Merged

elonsalfati merged 8 commits into
salfatigroup:mainfrom
techjacker:feat/testing-improvements

Conversation

@techjacker

@techjacker techjacker commented Mar 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary - v0.2.0

  • Add bailOnFailure config option to stop test run after first failure, saving LLM credits and CI time
  • Support granular recording: { trace: true, video: false } alongside the existing boolean, enabling trace-only recording when ffmpeg is unavailable (common in containers)
  • Add built-in ${VAR} / $VAR environment variable interpolation in YAML config files, eliminating the need for external tools like envsubst. Referencing an unset variable is a hard error — throws ConfigLoadError listing all missing variable names
  • Add viewportSize config option (default: 1280x720) to explicitly set browser viewport dimensions via --viewport-size flag, ensuring responsive web apps render at the expected breakpoint
  • Add secrets config option for masking sensitive values as *** in all reporter output. Secrets are specified as environment variable names (e.g. TEST_PASSWORD) — the loader resolves them to values via process.env, so the YAML never contains raw credentials
  • Add TEST_SKIPPED / "skipped" test status so the AI agent can skip tests when appropriate (e.g. a resource is already being processed). The agent's system prompt now includes TEST_SKIPPED as a valid response, and the runner handles it without retries or cache updates
  • Add displayName support — tests with a name field use it as the display label; unnamed tests show a truncated (80-char) version of the test case prompt. All reporter output uses the display name instead of the raw prompt text
  • Add message reporting — the agent's natural language response (TEST_PASSED: ..., TEST_FAILED: ..., TEST_SKIPPED: ...) is now captured as TestResult.message and shown in reporter output for all outcomes, giving per-test detail without repeating the full prompt
  • Fix per-test duration calculation — catch block in test runner now uses per-test start time instead of run-level start time
  • Fix recording paths in reporter — recordings section now shows just the recording directory instead of hardcoding trace.zip/video.webm filenames

Motivation

These features address common pain points when running OpenCheck in CI/CD pipelines:

  1. Wasted resources on known failures — without bail-on-failure, a 5-test suite continues burning LLM tokens after the first test fails (e.g. login failure means all subsequent tests will also fail)
  2. ffmpeg unavailability in containers — many CI environments and devcontainers don't have ffmpeg installed, making --save-video fail. Being able to record traces without video solves this
  3. External templating dependency — envsubst isn't available on all platforms (e.g. Alpine, Windows); built-in interpolation is more portable. Failing on unset variables prevents silent misconfiguration (e.g. an empty baseUrl from a missing $E2E_BASE_URL)
  4. Unpredictable viewport size — Playwright MCP's default viewport is narrow, causing responsive web apps to hide elements (columns, sidebars) that tests expect to find. Explicitly setting viewportSize ensures tests run against a known layout breakpoint
  5. Credential leakage in logs — test cases that include passwords (e.g. for login flows) show up verbatim in logs
  6. No way for the agent to skip tests — some test cases should be skipped based on runtime conditions (e.g. a resource is already being processed). Previously the agent could only pass or fail
  7. Noisy output — unnamed tests display the full multi-line prompt text in [RUNNING], [PASS]/[FAIL] lines, and the recordings section, making output hard to scan. Named tests and agent messages make output concise and informative

Changes

Config schema (src/config/schema.ts)

  • recording field now accepts boolean | { trace: boolean, video: boolean } via Zod union
  • Added bailOnFailure: boolean (default: false)
  • Added viewportSize: string (default: "1280x720", validated as WIDTHxHEIGHT format)
  • Added secrets: string[] (default: []) — values are environment variable names, not raw secrets

Config types (src/config/types.ts)

  • Added resolveRecording() helper to normalize boolean | object recording config to { trace, video }
  • Added RecordingOptions type export

Config loader (src/config/loader.ts)

  • Added interpolateEnvVars() that replaces ${VAR} and $VAR patterns with process.env values before YAML parsing
  • Unset variables throw a ConfigLoadError listing all missing variable names — fail-fast prevents silent misconfiguration
  • Added resolveSecrets() that maps env var names in secrets to their process.env values, filtering out any that are unset

MCP client (src/agent/mcp-client.ts)

  • Uses resolveRecording() to independently control --save-trace and --save-video flags
  • Passes --viewport-size from config to Playwright MCP, ensuring the browser opens at the specified dimensions

Agent types (src/agent/types.ts)

  • Added AgentOutcome type: "passed" | "failed" | "skipped"
  • AgentExecutionResult uses outcome: AgentOutcome as the sole status field (no legacy passed: boolean)

Agent factory (src/agent/agent-factory.ts)

  • System prompt now includes TEST_SKIPPED: <brief explanation> as a valid response alongside TEST_PASSED and TEST_FAILED
  • Response parsing detects TEST_SKIPPED and sets outcome: "skipped"

Test executor (src/runner/test-executor.ts)

  • Handles outcome === "skipped" — returns immediately without cache save, cache delete, or retry
  • Stores agentResult.message in TestResult.message for all outcomes (pass/fail/skip)

Runner types (src/runner/types.ts)

  • TestStatus now includes "skipped" alongside "passed" and "failed"
  • RunResult includes skipped count
  • TestResult includes displayName: string and optional message?: string

Test runner (src/runner/test-runner.ts)

  • Computes displayName from test.name (if set) or truncateTestCase(test.case) (80-char truncation)
  • Passes displayName and message to all reporter methods
  • Breaks out of the test loop after onTestComplete when bailOnFailure is true and test failed
  • aggregateResults counts skipped tests
  • Fixed per-test duration: catch block now uses testStartTime (set before each test) instead of run-level startTime

Reporter interface (src/output/types.ts)

  • onTestStart(displayName) — receives display name instead of raw test case
  • onTestComplete(displayName, status, source, durationMs, message?, error?) — receives display name and agent message

Reporter (src/output/reporter.ts)

  • Accepts optional SecretMasker in constructor
  • All output methods (onTestStart, onTestComplete, onRunComplete summary) apply masking before console.log
  • [SKIP] prefix for skipped tests
  • Skipped: count in summary table
  • Shows message (agent's response text) for all test outcomes under the status line
  • Shows error only when it differs from message (avoids duplication)
  • Failed tests summary and recordings section use displayName instead of raw testCase
  • Recordings section shows just Dir: <path> instead of hardcoded trace.zip/video.webm filenames

Secret masker (src/output/secret-masker.ts) — new file

  • Replaces secret values with *** using escaped regex patterns
  • Handles regex special characters in secrets safely

CLI (src/cli.ts)

  • Wires config.secrets (already resolved to values by loader) into SecretMasker → ConsoleReporter

Example config

baseUrl: "${E2E_BASE_URL}"
bailOnFailure: true
viewportSize: "1920x1080"
recording:
  trace: true
  video: false
secrets:
  - TEST_PASSWORD

tests:
  - name: "#login"
    case: >
      Navigate to ${E2E_BASE_URL} and sign in with ${TEST_USERNAME} / ${TEST_PASSWORD}
  - name: "Dashboard loads"
    case: >
      #login, then verify the dashboard has a table with columns OEM, JIRA Number, Processing Status.
  - case: >
      If the item is already being processed, skip this test.
      Otherwise, click "Process" and wait for completion.

Example output with name and message:

  [RUNNING] #login
  [PASS] #login (ai, 12.3s)
         TEST_PASSED: Successfully logged in via MS Entra popup.
  [RUNNING] Dashboard loads
  [PASS] Dashboard loads (cache, 1.2s)
  [RUNNING] If the item is already being processed, skip this test. Otherwise, click "Pr...
  [SKIP] If the item is already being processed, skip this test. Otherwise, click "Pr... (ai, 3.1s)
         TEST_SKIPPED: Item CV-3285 is already being processed.

Backwards compatibility

All new fields have defaults matching previous behavior:

  • bailOnFailure: false — tests continue after failure (existing behavior)
  • recording: true — both trace and video enabled (existing behavior); recording: { trace: true, video: false } is the new granular form
  • viewportSize: "1280x720" — reasonable default; can be overridden for apps with wider responsive breakpoints
  • secrets: [] — no masking (existing behavior)
  • Env var interpolation is transparent — configs without $ are unchanged
  • TEST_SKIPPED is opt-in — the agent only uses it when the test case instructions suggest skipping is appropriate. Existing test cases that only use TEST_PASSED / TEST_FAILED are unaffected
  • displayName defaults to a truncated version of testCase when no name is set — existing configs see cleaner output without changes
  • message is optional and only populated when the agent includes a response — no impact on configs that don't use it

Test plan

  • All 153 tests pass (bun run test)
  • TypeScript typecheck passes (tsc --noEmit)
  • Manual: run with bailOnFailure: true and a failing first test — verify remaining tests are skipped
  • Manual: run with recording: { trace: true, video: false } — verify trace captured, no video file
  • Manual: run with secrets: ["password"] and a test containing "password" — verify *** in output
  • Manual: run with an unset ${MISSING_VAR} in config — verify clear error message naming the variable
  • Manual: write a test case with skip instructions — verify [SKIP] status and Skipped: 1 in summary

techjacker and others added 5 commits March 31, 2026 07:11
… secret masking, and trace viewer control

Five new config options to improve CI/CD ergonomics:

1. `bailOnFailure` (bool, default false): stop test run after first failure to save LLM credits and CI time
2. `recording` now accepts `{ trace: bool, video: bool }` in addition to boolean, allowing trace-only recording (useful when ffmpeg is unavailable in containers)
3. `showTrace` (bool, default true): when false, passes `--no-show-trace` to Playwright MCP to prevent auto-launching trace viewer in headless/CI environments
4. Environment variable interpolation in YAML configs: `${VAR}` and `$VAR` syntax, replacing unset vars with empty string — eliminates the need for external `envsubst`
5. `secrets` (string[], default []): values are masked as "***" in all reporter output to prevent credentials from leaking into CI logs

All changes are backwards-compatible — existing configs work without modification.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@techjacker
techjacker marked this pull request as ready for review April 1, 2026 08:24
Comment thread src/agent/mcp-client.ts Outdated
Comment on lines +87 to +92
// Note: --no-show-trace is not yet supported by @playwright/mcp.
// When a future version adds this flag, uncomment to prevent
// auto-launching the trace viewer in headless/CI environments.
// if (config.showTrace === false) {
// playwrightArgs.push("--no-show-trace");
// }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this?

@techjacker techjacker Apr 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with 9c73819.

Comment thread src/agent/types.ts Outdated
Comment on lines 8 to 9
/** @deprecated Use `outcome` instead */
passed: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove deprecated code - it's still early enough in the project that I'm not worried about backward compatability

@techjacker techjacker Apr 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with 9c73819.

Comment thread src/config/schema.ts
bailOnFailure: z.boolean().default(false),
showTrace: z.boolean().default(true),
viewportSize: z.string().regex(/^\d+x\d+$/, "Must be WIDTHxHEIGHT, e.g. '1280x720'").default("1280x720"),
secrets: z.array(z.string()).default([]),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between secrets and just environment interpolation? What's the use-case for this?

Isn't there a case here where those are merged into a single concept and we just mask any env?

I'm mostly trying to think about it from the final config stand point - can we make it stupid simple that any AI / product manager can write those?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Difference

Environment interpolation (${VAR}) is for injection (getting values into the config), while secrets is for redaction (masking values in logs). You might interpolate a BASE_URL, which isn't a secret and should be visible in logs. However, a TEST_PASSWORD should be interpolated and masked.

Current implementation

secrets takes a list of env var names. The loader automatically resolves these to their values for the masker - see example config in PR description. We could potentially automatically mask any environment variable that is interpolated but some env vars (like URLs or browser types) are useful to see in logs.

Alternative

We could use a naming convention (e.g., any env var starting with SECRET_ is automatically masked) or a special interpolation syntax like ${SECRET:VAR_NAME}.

@elonsalfati

Copy link
Copy Markdown
Member

@techjacker - any thoughts on the comments above?

techjacker and others added 2 commits April 1, 2026 09:45
- Fix per-test duration calculation in `test-runner.ts` — the catch block was using the run-level `startTime` instead of a per-test start time, reporting incorrectly high durations for failed tests that weren't the first in the suite
- Fix recording paths in reporter output — previously always printed both `trace.zip` and `video.webm` paths even when video recording was disabled. Now shows just the recording directory, avoiding references to artifacts that don't exist

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@techjacker
techjacker force-pushed the feat/testing-improvements branch from 26bff67 to f78080d Compare April 1, 2026 09:53
@elonsalfati
elonsalfati merged commit 8f34b0e into salfatigroup:main Apr 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants