Skip to content

feat: add scheduled Playwright tests workflow with shard approach#10792

Open
paveltiunov wants to merge 3 commits intomasterfrom
cursor/scheduled-playwright-sharded-042a
Open

feat: add scheduled Playwright tests workflow with shard approach#10792
paveltiunov wants to merge 3 commits intomasterfrom
cursor/scheduled-playwright-sharded-042a

Conversation

@paveltiunov
Copy link
Copy Markdown
Member

@paveltiunov paveltiunov commented Apr 30, 2026

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made

Adds a new GitHub Actions workflow (.github/workflows/scheduled-playwright.yml) that runs Playwright tests on a daily schedule using the shard approach for parallelism.

Key features:

  • Scheduled execution: Runs daily at 06:00 UTC via cron
  • Manual trigger: Can also be triggered via workflow_dispatch
  • 8 shards on 8-core machines: Splits tests across 8 parallel runners using Playwright's built-in --shard flag on ubuntu-24.04-8core runners
  • Max concurrency of 4: Uses max-parallel: 4 so at most 4 shards run simultaneously
  • Report merging: A separate merge-reports job downloads all shard blob reports and merges them into a single HTML report artifact
  • Consistent patterns: Uses the same Node.js, Yarn, and cache setup as the existing CI workflows
  • Failure notifications: Posts to Slack on failure (consistent with other workflows)
Open in Web Open in Cursor 

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 30, 2026

Claude finished @paveltiunov's task in 1m 34s —— View job


Reviewing PR #10792

  • Read the new workflow file
  • Compare with existing CI workflow patterns
  • Review for code quality, security, performance, and best practices
  • Post comprehensive review

Review: .github/workflows/scheduled-playwright.yml

Overall the structure is solid — sharded matrix, report merging, consistent action versions, and Slack notifications follow the patterns in this repo. However, there are several issues ranging from a functional bug to an unused input that should be addressed before merging.


Bug: shard_count input is defined but never used

Lines 8-12 define a workflow_dispatch input shard_count with a default of '4', but it is never referenced anywhere in the workflow. The matrix is hardcoded to [1, 2, 3, 4] (line 22), and the shard total uses ${{ strategy.job-total }} (line 68).

This means manually triggering the workflow with a different shard_count value has no effect — the workflow always runs 4 shards regardless.

Options:

  • Remove the input if you always want 4 shards (simplest).
  • Wire it up using a dynamic matrix via fromJSON + a setup job, though that adds complexity.

Fix this →


Bug: Missing --reporter=blob — blob reports won't be generated

Line 68 runs:

npx playwright test --shard=${{ matrix.shard }}/${{ strategy.job-total }}

Without --reporter=blob, Playwright uses its default reporters (list/line). The blob-report/ directory (referenced in line 75) won't be created, so:

  • The artifact upload step will either fail or upload nothing
  • The merge-reports job will have nothing to merge

This should be:

npx playwright test --shard=${{ matrix.shard }}/${{ strategy.job-total }} --reporter=blob

Fix this →


Bug: Slack notification won't fire on Playwright test failures

The Slack step (line 110-118) is in the merge-reports job, which runs with if: always(). However, the failure() condition on the Slack step (line 111) evaluates the current job's step failures, not the upstream playwright job's status.

This means Slack only notifies if the report-download/merge steps themselves fail — not when Playwright tests fail, which is the primary case you want to alert on.

To fix, change line 111 from:

if: failure()

to:

if: ${{ always() && needs.playwright.result == 'failure' }}

Or alternatively, add a Slack notification step directly in the playwright job (using if: failure()), so each shard can independently notify.

Fix this →


Missing Playwright config

I found no playwright.config.ts or playwright.config.js anywhere in the repo. Without a config file, npx playwright test won't know which tests to run, which browser(s) to use, or what base URL to target. The sharded workflow will fail immediately.

Is the Playwright setup (config + tests) being added in a separate PR? If so, it may be worth noting that dependency here so the workflow isn't merged and running daily before the tests exist.


Minor: No PLAYWRIGHT_BLOB_OUTPUT_DIR or working directory

If Playwright tests live in a subdirectory (e.g., packages/cubejs-testing or a dedicated e2e/ directory), the workflow needs a working-directory on the test step. Currently everything runs from the repo root.


Positive notes

  • fail-fast: false is the right call for sharded test runs — you want all shards to complete so the merged report is comprehensive.
  • Retry logic on yarn install with nick-fields/retry@v3 matches the existing CI pattern.
  • retention-days: 1 for intermediate blobs vs. 14 for the merged report is a good balance.
  • timeout-minutes: 60 per shard is generous but reasonable for a daily schedule.
  • Slack notification pattern matches post-release.yml.

Summary

Three functional bugs need fixing before this workflow will work correctly: the missing --reporter=blob flag, the dead shard_count input, and the Slack notification condition. The missing Playwright config is also a blocker unless it's coming in a companion PR.

Comment thread .github/workflows/scheduled-playwright.yml Fixed
Comment thread .github/workflows/scheduled-playwright.yml Fixed
@paveltiunov paveltiunov marked this pull request as ready for review April 30, 2026 22:06
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 30, 2026

Claude finished @paveltiunov's task in 1m 23s —— View job


Reviewing PR #10792

  • Read the new workflow file
  • Compare with existing CI workflow patterns in the repo
  • Review for code quality, security, performance, and best practices
  • Check CodeQL security findings
  • Post comprehensive review

Review: .github/workflows/scheduled-playwright.yml

The overall structure is solid — sharded matrix, blob report merging, retry logic on install, and Slack notifications follow the patterns used across this repo's workflows. However, there are three functional bugs that will prevent the workflow from working correctly, plus a couple of smaller issues.


🔴 Bug: Missing --reporter=blob — blob reports won't be generated

Line 68 runs:

run: npx playwright test --shard=${{ matrix.shard }}/${{ strategy.job-total }}

Without --reporter=blob, Playwright uses its default reporters (list/line). The blob-report/ directory referenced in the upload step (line 75) will never be created, so:

  • The artifact upload will either fail or upload an empty artifact
  • The merge-reports job will have nothing to merge

Fix:

run: npx playwright test --shard=${{ matrix.shard }}/${{ strategy.job-total }} --reporter=blob

Fix this →


🔴 Bug: Slack notification won't fire on Playwright test failures

The Slack step (line 110-118) is in the merge-reports job. The failure() condition on line 111 evaluates the current job's step failures, not the upstream playwright job's status.

This means Slack only notifies if the report-download/merge steps themselves fail — not when Playwright tests fail, which is the primary use case.

Fix — change line 111 from:

if: failure()

to:

if: ${{ always() && needs.playwright.result == 'failure' }}

Fix this →


🟡 Dead code: shard_count input is defined but never used

Lines 8-12 define a workflow_dispatch input shard_count with a default of '4', but it's never referenced. The matrix is hardcoded to [1, 2, 3, 4] (line 22), and the shard denominator uses ${{ strategy.job-total }} (line 68).

Manually triggering with a different shard_count value has no effect — it always runs 4 shards.

Options:

  • Remove the input if 4 shards is always desired (simplest, recommended).
  • Wire it up dynamically using fromJSON + a setup job (adds complexity).

Fix this →


🟡 Missing Playwright config

There is no playwright.config.ts or playwright.config.js anywhere in the repo. Without a config file, npx playwright test won't know which tests to run, which browser(s) to use, or what base URL to target. The workflow will fail immediately when run.

Is the Playwright setup (config + test files) being added in a companion PR? If so, it's worth noting that dependency here so this workflow isn't merged and running (and failing) daily before the tests exist.


🟡 Security: Missing permissions block (CodeQL finding)

CodeQL flagged both jobs (finding 1, finding 2) for not limiting GITHUB_TOKEN permissions. This workflow only needs read access to the repo, so adding a top-level permissions block would follow least-privilege:

permissions:
  contents: read

Note: Most of the existing workflows in this repo (master.yml, pr.yml, post-release.yml) also omit top-level permissions, so this is consistent with the current pattern. However, it's still best practice to add it — especially for a scheduled workflow, where the default token gets write access even on forks.


Minor observations

Item Details
fail-fast: false Correct for sharded test runs — all shards should complete for a comprehensive merged report
✅ Retry on yarn install Matches the nick-fields/retry@v3 pattern used in other workflows
retention-days: 1 vs 14 Good balance: ephemeral blobs vs. longer-lived merged report
timeout-minutes: 60 Reasonable for a daily scheduled run
⚠️ No working-directory If Playwright tests end up in a subdirectory (e.g., packages/cubejs-testing/), the test step will need a working-directory
⚠️ Full yarn build && yarn tsc This builds the entire monorepo. If Playwright tests only need a subset, consider scoping the build to reduce CI time

Summary

Three functional bugs need fixing before this workflow will operate correctly:

  1. Missing --reporter=blob — reports won't be generated for merging
  2. Slack condition — won't notify on actual test failures
  3. Dead shard_count input — misleading but harmless

The missing Playwright config is also a blocker unless it's coming in a separate PR. The CodeQL permissions finding is worth addressing but not blocking.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Comment thread .github/workflows/scheduled-playwright.yml Fixed
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@cursor cursor Bot deleted the cursor/scheduled-playwright-sharded-042a branch April 30, 2026 22:54
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