feat(ci): Add Windows runner for Playwright E2E tests#7769
feat(ci): Add Windows runner for Playwright E2E tests#7769SBALAVIGNESH123 wants to merge 3 commits intoComfy-Org:mainfrom
Conversation
dosubot
bot
added
the
size:M
📝 WalkthroughWalkthroughAdds a new GitHub Actions job Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions (windows-latest)
participant Repo as Repository (actions/checkout)
participant Server as ComfyUI Server
participant Node as Node.js / pnpm / Frontend Build
participant Playwright as Playwright Runner (Chromium)
participant Reports as Report Merger/Generator
participant Artifacts as Artifact Storage
Note over GH: Start job `playwright-tests-windows` (matrix shard)
GH->>Repo: checkout repository
GH->>Server: start ComfyUI server (launch_server: true)
GH->>Node: setup Node.js & pnpm, build frontend (no cache)
GH->>Playwright: install/setup Playwright
Playwright->>Server: run Chromium tests (shard)
Playwright->>Artifacts: write blob report `blob-report-windows-<shard>`
Artifacts->>Reports: download all `blob-report-*`
Reports->>Reports: merge JSON blobs → generate HTML/JSON reports
Reports->>Artifacts: upload merged report artifacts
Note over GH,Artifacts: Job matrix shards complete
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status
⏰ Started at: 12/28/2025, 07:58:40 AM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
🎭 Playwright Test Results
⏰ Started at: 12/28/2025, 07:58:40 AM UTC 🚀 Running Tests
⏱️ Please wait while tests are running... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
266-267: Windows test results won't block PR deployment.The
deploy-and-commentjob'sneedsarray doesn't includeplaywright-tests-windows, meaning deployment will proceed even if Windows tests fail or are still running. This undermines the value of adding Windows CI coverage.🔎 Proposed fix: Add Windows job to deployment dependencies
deploy-and-comment: - needs: [playwright-tests, merge-reports] + needs: [playwright-tests, merge-reports, playwright-tests-windows] runs-on: ubuntu-latest
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-tests-e2e.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to browser_tests/**/*.spec.ts : Use E2E tests in `browser_tests/**/*.spec.ts` with Playwright framework
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Tests should be cross-platform compatible using `path.resolve`, `path.join`, and `path.sep` for Windows, macOS, and Linux compatibility
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-21T06:04:12.562Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to browser_tests/**/*.spec.ts : Use E2E tests in `browser_tests/**/*.spec.ts` with Playwright framework
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
🔇 Additional comments (3)
.github/workflows/ci-tests-e2e.yaml (3)
167-174: Verify composite actions are Windows-compatible.The composite actions used here (
./.github/actions/setup-comfyui-server,setup-frontend,setup-playwright) must support Windows runners. Verify they:
- Use cross-platform commands or include Windows-specific steps
- Handle Windows path separators correctly
- Support Windows-specific Python/Node.js installation if needed
- Can launch the ComfyUI server on Windows
The verification script in the previous comment will display the action definitions to confirm Windows compatibility.
157-159: Good practice: Using bash shell on Windows for consistency.Explicitly specifying
shell: bashensures cross-platform compatibility of the commands and is supported by thewindows-latestrunner, which includes Git Bash.
191-197: Good artifact naming prevents conflicts.The artifact name
playwright-report-windows-chromiumis distinct from the merged Linux chromium report (playwright-report-chromiumat line 232) and will be correctly matched by the download patternplaywright-report-*at line 280.
|
Ready for CI approval! I've implemented the Windows inputs as requested. Let me know if you need any other tweaks. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci-tests-e2e.yaml (1)
164-173: Composite actions have critical Windows compatibility issues.The three composite actions have Unix-specific commands that will fail on Windows runners:
setup-comfyui-server: Usescp -rcommand (not available on Windows cmd/PowerShell) andwait-for-itutility (Unix-specific shell script)setup-playwright: Usesjqfor JSON parsing (unavailable on Windows) and Unix-style cache path~/.cache/ms-playwrightAll actions explicitly specify
shell: bash, which may not be available on default Windows runners. Replace Unix-only commands with cross-platform alternatives:
- Replace
cp -rwith Node.js-based copy or use@actions/coreutilities- Replace
wait-for-itwith cross-platform polling (Node.js or Python)- Replace
jqwithnode -eor built-in JSON parsingAlternatively, add explicit Windows conditionals or ensure bash is available system-wide on Windows runners.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-tests-e2e.yaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to browser_tests/**/*.spec.ts : Use E2E tests in `browser_tests/**/*.spec.ts` with Playwright framework
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
📚 Learning: 2025-12-12T23:02:37.473Z
Learnt from: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 7422
File: .github/workflows/pr-update-playwright-expectations.yaml:131-135
Timestamp: 2025-12-12T23:02:37.473Z
Learning: In the `.github/workflows/pr-update-playwright-expectations.yaml` workflow in the Comfy-Org/ComfyUI_frontend repository, the snapshot update process is intentionally scoped to only add and update snapshot images. Deletions of snapshot files are handled explicitly outside this workflow and should not be suggested as part of this automation.
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-21T06:04:12.562Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to browser_tests/**/*.spec.ts : Use E2E tests in `browser_tests/**/*.spec.ts` with Playwright framework
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Tests should be cross-platform compatible using `path.resolve`, `path.join`, and `path.sep` for Windows, macOS, and Linux compatibility
Applied to files:
.github/workflows/ci-tests-e2e.yaml
📚 Learning: 2025-12-21T06:04:12.562Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to browser_tests/**/*.spec.ts : Use tags like `mobile` and `2x` in Playwright tests for relevant test variations; tags are respected by config
Applied to files:
.github/workflows/ci-tests-e2e.yaml
🔇 Additional comments (4)
.github/workflows/ci-tests-e2e.yaml (4)
150-191: LGTM! Past feedback correctly addressed.The Windows job implementation properly addresses all previous concerns:
- No cross-platform cache restoration (removed
needs: setupdependency)- Explicit Windows build (
include_build_step: true)- 8-way sharding implemented to prevent timeouts
- Unique artifact naming per shard
The 20-minute timeout is reasonable with sharding in place.
178-178: Verify that testing only chromium project on Windows is intentional.The Windows job runs only
--project=chromium, while theplaywright-testsjob (lines 92-148) also testschromium-2x,chromium-0.5x, andmobile-chromeprojects on Linux. This appears aligned with issue #3197 ("Run the Chromium Playwright project on the Windows runner"), but please confirm that omitting the other browser/scaling variants from Windows testing is intentional.
194-194: LGTM! Merge dependencies and pattern correctly updated.The
merge-reportsjob now:
- Waits for Windows tests to complete (line 194)
- Collects both Linux and Windows blob reports via the updated
blob-report-*pattern (line 211)This ensures Windows test results are included in the merged HTML report.
Also applies to: 211-211
260-260: LGTM! Deployment now waits for Windows tests.The
deploy-and-commentjob correctly includesplaywright-tests-windowsin its dependencies, ensuring Windows test failures block deployment. This addresses the previous feedback and aligns with the cross-platform validation objectives from issue #3197.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| shard: [1, 2, 3, 4, 5, 6, 7, 8] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider renaming for consistency.
The Linux chromium job uses matrix.shardIndex (line 54), while this Windows job uses matrix.shard. For maintainability, consider using shardIndex here as well.
🔎 Proposed change for consistency
matrix:
- shard: [1, 2, 3, 4, 5, 6, 7, 8]
+ shardIndex: [1, 2, 3, 4, 5, 6, 7, 8]Then update references on lines 178 and 188:
- pnpm exec playwright test --project=chromium --reporter=blob --shard=${{ matrix.shard }}/8
+ pnpm exec playwright test --project=chromium --reporter=blob --shard=${{ matrix.shardIndex }}/8- name: blob-report-windows-${{ matrix.shard }}
+ name: blob-report-windows-${{ matrix.shardIndex }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/ci-tests-e2e.yaml around line 159: the Windows job defines the
shard matrix key as "shard" while the Linux job uses "shardIndex", causing
inconsistency; rename the matrix key from "shard" to "shardIndex" and update any
references that use matrix.shard to matrix.shardIndex (notably at lines 178 and
188) so the variable name is consistent across jobs.
| uses: actions/upload-artifact@v4 | ||
| if: always() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align upload condition with Linux sharded job.
The Windows job uses if: always(), but the Linux chromium job uses if: ${{ !cancelled() }} (line 86). The !cancelled() condition is more appropriate as it uploads reports on success or failure but skips uploading if the workflow is cancelled, saving storage costs.
🔎 Proposed fix
- name: Upload Blob Report
uses: actions/upload-artifact@v4
- if: always()
+ if: ${{ !cancelled() }}
with:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/upload-artifact@v4 | |
| if: always() | |
| uses: actions/upload-artifact@v4 | |
| if: ${{ !cancelled() }} |
🤖 Prompt for AI Agents
In .github/workflows/ci-tests-e2e.yaml around lines 184-185 the Windows job uses
"if: always()", which uploads artifacts even when the workflow is cancelled;
change the condition to "if: ${{ !cancelled() }}" to match the Linux chromium
job and ensure artifacts are uploaded on success or failure but skipped when the
workflow is cancelled.
|
Thanks for the contribution! After reviewing this, the decision is not to move forward with it at this time for a few reasons:
This may be revisited in the future if the need arises. Thanks again for putting this together! |
2 participants
This PR adds a Windows runner job to the E2E test workflow to ensure cross-platform compatibility for browser tests.
Fixes #3197
Changes:
┆Issue is synchronized with this Notion page by Unito