Skip to content

ci: DH-22941: Record new e2e tests and link to the videos from the PR - #2740

Open
mofojed wants to merge 7 commits into
deephaven:mainfrom
mofojed:DH-22941-auto-e2e-output-action
Open

ci: DH-22941: Record new e2e tests and link to the videos from the PR#2740
mofojed wants to merge 7 commits into
deephaven:mainfrom
mofojed:DH-22941-auto-e2e-output-action

Conversation

@mofojed

@mofojed mofojed commented Aug 20, 2026

Copy link
Copy Markdown
Member
  • Create a new action npm run e2e:record which records an e2e test run in slowMo of the file specified
    • Automatically trims off the starting white screen/loading spinner, so it jumps right into the test
  • Add a GitHub action to add a comment to any PR with new e2e tests linking to videos of those tests
  • See an example of the link generated from a PR in my fork: test: add e2e tests for the settings menu mofojed/web-client-ui#23 (comment)

Adds `npm run e2e:record <file-name> [search-name]` which runs a single e2e
test with video always on, trims the leading dev-server load screen with
ffmpeg, and exports an mp4 for attaching to a PR or Jira ticket.
Adds a `record-new-tests` job to the e2e workflow that runs in parallel with
the test matrix. It diffs the PR for added or modified `tests/*.spec.ts`
files, records them in chromium with the recording config, uploads the videos
as an artifact, and posts a sticky comment linking to it.

- Add playwright-record-ci.config.ts, which pairs the recording config with
  the CI preview servers
- Support multiple spec files, a --grep flag, and an E2E_RECORD_CONFIG
  override in scripts/e2e-record.mjs
- Install ffmpeg in the e2e image so recordings are trimmed and exported to mp4
- Add an e2e-record docker compose service
@mofojed
mofojed requested a review from dsmmcken August 20, 2026 20:54
@mofojed mofojed self-assigned this Aug 20, 2026
Copilot AI balanced review requested due to automatic review settings August 20, 2026 20:54
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.84%. Comparing base (251c39a) to head (7a5c628).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2740      +/-   ##
==========================================
+ Coverage   51.39%   51.84%   +0.45%     
==========================================
  Files         797      797              
  Lines       45887    45928      +41     
  Branches    11733    11753      +20     
==========================================
+ Hits        23582    23811     +229     
+ Misses      22286    22097     -189     
- Partials       19       20       +1     
Flag Coverage Δ
unit 51.84% <ø> (+0.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds automated Playwright video recording and PR artifact links for changed e2e specifications.

Changes:

  • Adds recording-specific Playwright configurations and server warm-up.
  • Generates trimmed MP4 recordings and an HTML index.
  • Adds Docker and GitHub Actions automation for recording changed specs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
.github/workflows/e2e.yml Records changed specs and comments with artifacts.
package.json Adds the recording command.
playwright-record-ci.config.ts Adapts recording configuration for CI.
playwright-record.config.ts Configures serial Chromium video recording.
scripts/e2e-record.mjs Runs tests, trims videos, and builds the report.
tests/docker-scripts/Dockerfile Installs ffmpeg.
tests/docker-scripts/docker-compose.yml Adds the recording service.
tests/record-globalSetup.ts Warms the server before recording.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/e2e.yml Outdated
Comment thread playwright-record.config.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 21, 2026 13:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/e2e-record.mjs:177

  • slice returns fewer than CONTENT_FRAME_RUN frames near the end, and every still succeeds for a one- or two-frame suffix. An isolated high-energy frame at the end can therefore be treated as sustained content and cause almost the entire recording to be trimmed. Require the full three-frame window to exist before testing its energy.
  const firstContent = frames.findIndex((_, i) =>
    frames
      .slice(i, i + CONTENT_FRAME_RUN)
      .every(frame => frame.energy >= threshold)
  );

scripts/e2e-record.mjs:392

  • Playwright cleans outputDir at the start of every run, so every file collected afterward belongs to the current run even when its pathname was seen previously. On a repeated local run that produces both an existing pathname and a new one, newVideos is non-empty and the existing-path recording is omitted from index.html. Drop the pre-run pathname filter and report all videos left in the output directory.
const allVideos = collectVideos(outputDir);
const newVideos = allVideos.filter(v => !previousVideos.has(v));
const videos = newVideos.length > 0 ? newVideos : allVideos;

dsmmcken
dsmmcken previously approved these changes Aug 21, 2026
Copilot AI review requested due to automatic review settings August 21, 2026 19:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/e2e-record.mjs:177

  • slice returns fewer than CONTENT_FRAME_RUN frames near the end, and every still succeeds for a one- or two-frame tail. A recording that never sustains content for three frames can therefore be trimmed down to its final transient frames, despite the guard below intending to leave such runs untrimmed. Require i + CONTENT_FRAME_RUN <= frames.length before checking the slice.
  const firstContent = frames.findIndex((_, i) =>
    frames
      .slice(i, i + CONTENT_FRAME_RUN)
      .every(frame => frame.energy >= threshold)
  );

.github/workflows/e2e.yml:177

  • This only deletes the sticky comment when there are no changed specs. If recording or upload produces no files, artifact-url is empty, the update step is skipped, and the previous run's comment remains with an outdated spec list and artifact link. Delete the sticky comment whenever the upload has no artifact URL (while retaining the same-repository and cancellation guards).
      - name: Remove stale recordings comment
        if: ${{ steps.specs.outputs.spec-files == '' && github.event.pull_request.head.repo.full_name == github.repository }}

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.

3 participants