Add QEMU E2E CI workflow with Playwright tests for ESP32#303
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR implements a complete QEMU-based end-to-end testing infrastructure for WLED's ESP32 firmware. It adds helper scripts to set up and run QEMU, configures a QEMU-specific firmware build, guards WiFi/AP initialization and network access in the firmware with ChangesQEMU E2E Testing Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
- Add Playwright dependency and test scripts - Create E2E tests for index, settings, and other pages - Add QEMU ESP32 setup and run scripts - Create GitHub Actions workflow for QEMU testing - Update tests to work with ESP32 backend (not static files) - Configure proper timeouts for QEMU emulation Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
- Remove mock server (doesn't match requirements) - Finalize QEMU ESP32 setup and run scripts - Complete Playwright test suite for all pages - Update GitHub Actions workflow for QEMU testing - All pages served from actual ESP32 firmware in QEMU - Tests verify no JavaScript errors on page load Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
The old URL (esp-develop-20220919) was returning 404 errors. Updated to use the latest stable release with correct URL format: - Release tag includes date suffix (esp-develop-9.2.2-20250817) - File path includes version in filename - Binary location changed to bin/qemu-system-xtensa - Added fallback URLs for older versions - Added symlink creation for backward compatibility Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
@copilot is it possible to run the e2e test with gcc sanitizing features enabled? Especially ThreadSanitizer would be interesting. |
ThreadSanitizer and other GCC sanitizers have limited support for ESP32: Challenges:
Potential alternatives:
If you want to proceed, I can:
However, this is experimental and may not work reliably. Would you like me to attempt it, or would you prefer focusing on other testing approaches? |
This comment was marked as resolved.
This comment was marked as resolved.
Added step to install required shared libraries before setting up QEMU: - libsdl2-2.0-0 (SDL2 library - required by QEMU) - libpixman-1-0 (pixel manipulation library) - libglib2.0-0 (GLib library) Enhanced setup-qemu.sh to verify dependencies after installation: - Checks for missing libraries with ldd - Shows QEMU version if successful - Provides clear error message with install commands if libraries missing This fixes the "error while loading shared libraries: libSDL2-2.0.so.0" error. Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Port 80 is a privileged port requiring root permissions, which CI jobs don't have and shouldn't require. Changed to use port 8080 instead: - Updated workflow to use port 8080 for QEMU - Changed default port in run-qemu.sh from 80 to 8080 - Updated all test URLs to use localhost:8080 - Updated README documentation with correct port - Updated Playwright config default baseURL to use port 8080 This allows QEMU to start without requiring root/sudo privileges. Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
The CI workflow calls 'npm run test:e2e' but the script was not defined in package.json. Added it to run 'npx playwright test'.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/openeth_mac/openeth.h (1)
152-152: ⚡ Quick winConsider adding WLEDMM marker for C++ compatibility fix.
The
or_flagfield was renamed fromorto avoid a C++ keyword conflict. Per coding guidelines, WLED-MM-specific modifications to vendored code should be marked with// WLEDMMcomments to track local changes.📝 Suggested annotation
- uint16_t or_flag: 1; //!< Overrun flag (renamed from 'or' to avoid C++ keyword conflict) + uint16_t or_flag: 1; //!< Overrun flag (renamed from 'or' to avoid C++ keyword conflict) // WLEDMM C++ compatibility🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/openeth_mac/openeth.h` at line 152, The field name or_flag was changed from the original 'or' to avoid a C++ keyword conflict but the change lacks the required WLED-MM marker; update the declaration of uint16_t or_flag: 1; in openeth.h to include a trailing comment like // WLEDMM (or the project’s exact marker) so the local vendored modification is clearly annotated for tracking and C++ compatibility (reference symbol: or_flag).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/openeth_mac/openeth.h`:
- Line 152: The field name or_flag was changed from the original 'or' to avoid a
C++ keyword conflict but the change lacks the required WLED-MM marker; update
the declaration of uint16_t or_flag: 1; in openeth.h to include a trailing
comment like // WLEDMM (or the project’s exact marker) so the local vendored
modification is clearly annotated for tracking and C++ compatibility (reference
symbol: or_flag).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a05136e-2ad7-4eaf-83d6-2cd3f7395379
📒 Files selected for processing (10)
.github/workflows/qemu-e2e-test.yml.gitignoree2e-tests/README.mdlib/openeth_mac/esp_eth_mac_openeth.clib/openeth_mac/library.jsonlib/openeth_mac/openeth.hplatformio.inisdkconfig.defaults.qemuwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (2)
- wled00/wled.h
- wled00/wled.cpp
✅ Files skipped from review due to trivial changes (3)
- sdkconfig.defaults.qemu
- .gitignore
- e2e-tests/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/qemu-e2e-test.yml
- platformio.ini
|
@netmindz We finally have a working toolchain !! 😀 Took only 6 month to get here with AI support (so much about "10x developers" 😜 ) |
|
@copilot please analyze the log files i've uploaded in comment #303 (comment) . It looks like the playwright tests are trying to access the WLED-MM WebUI pages directly, however the server routes from https://github.com/MoonModules/WLED-MM/blob/mdev/wled00/wled_server.cpp should be used: './', .'/settings/', './settings/sync', etc. (littleFS is initialized and functional, however web pages are directly served from the WLED firmware image). Please iterate over the e2e test script until we have a working test that pulls all main UI elements (see original expectations in #302 ). |
The tests were using filesystem-based paths like /index.htm and /settings_wifi.htm which only work with LittleFS content. WLED serves pages from compiled-in firmware data via route-based URLs: - '/' for main UI (not /index.htm) - '/settings' for settings landing - '/settings/wifi', '/settings/leds', etc. for sub-pages - '/welcome', '/liveview', '/update' for other pages - '/json/info', '/json/state' for API endpoints Also added JSON API validation tests.
Fixed in the latest commit. The tests were using filesystem-based paths (
Updated tests:
All pages verified for JavaScript uncaught exceptions (page errors). |
- Create shared Playwright fixture that captures all browser console messages (log, warn, error, info, debug) and page errors per test - Write logs to console-logs/ directory with one file per test - Attach logs to Playwright HTML report for inline viewing - Upload console-logs/ as separate CI artifact (7-day retention) - Add console-logs/ to .gitignore
Yes — JS console logs are now captured and available as CI artifacts. The implementation:
Example log output for a test: This will capture messages like the |
This comment was marked as resolved.
This comment was marked as resolved.
Settings pages have individual titles like "Usermod Settings", "2D Set-up", "Wi-Fi Settings" etc. - not all contain "WLED". Updated tests to expect the actual title served by each page.
... Fixed in the latest commit. The issue was that
Updated the tests to expect the exact title each page actually serves (sourced from |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/qemu-e2e-test.yml (1)
94-96: ⚡ Quick winRemove redundant
npm install@playwright/test``.If
@playwright/testis listed inpackage.json(which it should be per the PR),npm cion line 91 already installs it. The explicitnpm install@playwright/test`` creates a duplicate installation and may cause version drift if the package.json version differs.Suggested fix
- name: Install Playwright Browsers run: | - npm install `@playwright/test` npx playwright install --with-deps chromium#!/bin/bash # Verify `@playwright/test` is in package.json dependencies cat package.json | jq '.devDependencies["`@playwright/test`"] // .dependencies["`@playwright/test`"]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qemu-e2e-test.yml around lines 94 - 96, Remove the redundant explicit "npm install `@playwright/test`" in the workflow run block and rely on the earlier "npm ci" step to install dependencies; keep the "npx playwright install --with-deps chromium" line so Playwright browsers are installed, and ensure only the literal "npm install `@playwright/test`" invocation is removed from the run block to avoid duplicate installs and potential version drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qemu-e2e-test.yml:
- Line 7: Remove the temporary development branch from the push trigger in the
workflow file by editing the branches array in
.github/workflows/qemu-e2e-test.yml: delete the
"copilot/add-ci-workflow-for-esp32" entry so the branches list only contains the
intended persistent branches (e.g., "mdev" and "main"), then commit the change.
In `@package.json`:
- Line 13: The repo runs Playwright via the "test:e2e" script but does not
declare the Playwright packages, making installs non-deterministic; add
`@playwright/test` as a devDependency in package.json (and optionally the
top-level playwright package if you need browser binaries) so files that import
it (playwright.config.js, e2e-tests/fixtures.js, e2e-tests/index.spec.js)
resolve deterministically; update package.json devDependencies and run npm/yarn
install to lock the dependency.
---
Nitpick comments:
In @.github/workflows/qemu-e2e-test.yml:
- Around line 94-96: Remove the redundant explicit "npm install
`@playwright/test`" in the workflow run block and rely on the earlier "npm ci"
step to install dependencies; keep the "npx playwright install --with-deps
chromium" line so Playwright browsers are installed, and ensure only the literal
"npm install `@playwright/test`" invocation is removed from the run block to avoid
duplicate installs and potential version drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f95ea042-5177-444f-8baf-bc0589a5d8ec
📒 Files selected for processing (16)
.github/workflows/qemu-e2e-test.yml.gitignoree2e-tests/IMPLEMENTATION_SUMMARY.mde2e-tests/README.mde2e-tests/fixtures.jse2e-tests/index.spec.jse2e-tests/other-pages.spec.jse2e-tests/settings.spec.jslib/openeth_mac/esp_eth_mac_openeth.clib/openeth_mac/library.jsonlib/openeth_mac/openeth.hpackage.jsonplatformio.inisdkconfig.defaults.qemuwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (3)
- e2e-tests/IMPLEMENTATION_SUMMARY.md
- wled00/wled.h
- wled00/wled.cpp
✅ Files skipped from review due to trivial changes (2)
- sdkconfig.defaults.qemu
- e2e-tests/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e-tests/other-pages.spec.js
- .gitignore
- e2e-tests/settings.spec.js
👮 Files not reviewed due to content moderation or server errors (4)
- lib/openeth_mac/openeth.h
- lib/openeth_mac/esp_eth_mac_openeth.c
- lib/openeth_mac/library.json
- platformio.ini
| pull_request: | ||
| branches: [ mdev, main ] | ||
| push: | ||
| branches: [ mdev, main, copilot/add-ci-workflow-for-esp32 ] |
There was a problem hiding this comment.
Remove temporary development branch from push trigger before merging.
The copilot/add-ci-workflow-for-esp32 branch appears to be a development branch for this PR. It should be removed from the push triggers before merging to avoid running this workflow on unrelated pushes.
Suggested fix
push:
- branches: [ mdev, main, copilot/add-ci-workflow-for-esp32 ]
+ branches: [ mdev, main ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/qemu-e2e-test.yml at line 7, Remove the temporary
development branch from the push trigger in the workflow file by editing the
branches array in .github/workflows/qemu-e2e-test.yml: delete the
"copilot/add-ci-workflow-for-esp32" entry so the branches list only contains the
intended persistent branches (e.g., "mdev" and "main"), then commit the change.
| "scripts": { | ||
| "build": "node tools/cdata.js", | ||
| "test": "node --test", | ||
| "test:e2e": "npx playwright test", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dependency declaration vs actual usage.
set -euo pipefail
echo "Playwright usage in repo:"
rg -n --type=js "`@playwright/test`|playwright test|page\\.request\\.get\\("
echo
echo "Playwright declarations in package.json:"
rg -n "\"`@playwright/test`\"|\"playwright\"|\"test:e2e\"" package.jsonRepository: MoonModules/WLED-MM
Length of output: 498
Declare the Playwright test runner as a devDependency to keep installs deterministic.
test:e2e runs npx playwright test, while the repo directly imports @playwright/test (e.g., playwright.config.js, e2e-tests/fixtures.js, and e2e-tests/index.spec.js via page.request.get(...)). package.json only contains the test:e2e script and has no @playwright/test (or playwright) dependency, so CI/local runs depend on transient npx resolution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 13, The repo runs Playwright via the "test:e2e" script
but does not declare the Playwright packages, making installs non-deterministic;
add `@playwright/test` as a devDependency in package.json (and optionally the
top-level playwright package if you need browser binaries) so files that import
it (playwright.config.js, e2e-tests/fixtures.js, e2e-tests/index.spec.js)
resolve deterministically; update package.json devDependencies and run npm/yarn
install to lock the dependency.
This comment was marked as resolved.
This comment was marked as resolved.
... Both errors are expected firmware behavior in the QEMU build, not CI firewall issues: 1. 2. Neither error causes JavaScript exceptions (the tests pass). They're just browser-reported network errors for resources that can't be served in this configuration. The No action needed — these are informational console messages, not bugs. |
- enable OTA page - enable simplified UI (optional) - enable PixelForge
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e-tests/index.spec.js (1)
1-80: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMark AI-generated test suite with attribution comments.
Per coding guidelines for AI-generated JavaScript, this test file should be marked with
// AI: below section was generated by an AIand// AI: endcomments to document its origin.📝 Suggested addition
+// AI: below section was generated by an AI // `@ts-check` const { test, expect } = require('./fixtures'); /** * Test that the main index page loads without JavaScript errors. * The WLED web UI is served from compiled-in firmware data via the '/' route. * The '/index.htm' path only serves from LittleFS (custom uploaded UI) and * won't work when there's no filesystem content. */ test.describe('WLED Index Page', () => { // ... tests ... }); +// AI: end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` around lines 1 - 80, Add AI attribution comments surrounding the generated test suite: insert a leading comment "// AI: below section was generated by an AI" immediately before the block that begins with test.describe('WLED Index Page', ...) and a trailing comment "// AI: end" immediately after its closing "});" so the entire suite (all tests within test.describe) is clearly marked as AI-generated; ensure the comments are plain line comments at the top and bottom of the described block so tooling and reviewers can identify the generated section.Source: Coding guidelines
e2e-tests/fixtures.js (1)
1-53: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMark AI-generated code with attribution comments.
Per coding guidelines for AI-generated JavaScript, this fixture should be marked with
// AI: below section was generated by an AIand// AI: endcomments to document its origin.📝 Suggested addition
+// AI: below section was generated by an AI // `@ts-check` const base = require('`@playwright/test`'); const fs = require('fs'); const path = require('path'); const CONSOLE_LOG_DIR = path.join(__dirname, '..', 'console-logs'); // Ensure the console-logs directory exists if (!fs.existsSync(CONSOLE_LOG_DIR)) { fs.mkdirSync(CONSOLE_LOG_DIR, { recursive: true }); } /** * Extended test fixture that captures all browser console messages * and saves them as downloadable CI artifacts. */ exports.test = base.test.extend({ page: async ({ page }, use, testInfo) => { const consoleLogs = []; // Capture ALL console messages (log, warn, error, info, debug) page.on('console', msg => { const entry = `[${msg.type().toUpperCase()}] ${msg.text()}`; consoleLogs.push(entry); }); // Capture uncaught page errors page.on('pageerror', error => { consoleLogs.push(`[PAGE_ERROR] ${error.message}`); }); await use(page); // After test completes, write console logs to file if (consoleLogs.length > 0) { const safeName = testInfo.titlePath.join(' - ') .replace(/[^a-zA-Z0-9_\-. ]/g, '_') .replace(/\s+/g, '_'); const logFile = path.join(CONSOLE_LOG_DIR, `${safeName}.log`); const logContent = consoleLogs.join('\n') + '\n'; fs.writeFileSync(logFile, logContent); // Also attach to test report for visibility in Playwright HTML report await testInfo.attach('console-logs', { body: logContent, contentType: 'text/plain', }); } }, }); exports.expect = base.expect; +// AI: end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/fixtures.js` around lines 1 - 53, Add AI attribution comment markers around the generated fixture block so it's clearly documented as AI-generated; specifically insert a leading comment like "// AI: below section was generated by an AI" immediately before the exports.test = base.test.extend({ ... }) block and a trailing comment like "// AI: end" immediately after the end of that block (after the closing });). Ensure the markers surround the entire console-capturing logic that references CONSOLE_LOG_DIR, the page fixture listener setup (page.on('console', ...), page.on('pageerror', ...)), and the testInfo.attach/writeFileSync cleanup so the AI origin is obvious while leaving exports.expect and other code unchanged.Source: Coding guidelines
♻️ Duplicate comments (2)
package.json (1)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
@playwright/testas a devDependency.The
test:e2escript runsnpx playwright test, and multiple files import from@playwright/test(playwright.config.js,e2e-tests/fixtures.jsat line 2, and transitivelye2e-tests/index.spec.jsvia fixtures), butpackage.jsondoes not declare@playwright/testindevDependencies. This makes local and CI installs non-deterministic becausenpxfetches the package on-demand while the import statements require it to be installed innode_modules.📦 Add devDependency
"dependencies": { "clean-css": "^5.3.3", "html-minifier-terser": "^7.2.0", "web-resource-inliner": "^7.0.0", "nodemon": "^3.1.9" + }, + "devDependencies": { + "`@playwright/test`": "^1.48.2" }Then run
npm installto update the lock file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 13, The project uses `@playwright/test` (imported by playwright.config.js, e2e-tests/fixtures.js and e2e-tests/index.spec.js) but package.json only defines the "test:e2e" script calling npx; add "`@playwright/test`" to package.json devDependencies so the package is installed in node_modules instead of fetched on-demand, then run npm install to update package-lock.json (or yarn.lock) so CI and local installs are deterministic.e2e-tests/index.spec.js (1)
28-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert navigation responses are successful before proceeding.
Both
page.goto('/')calls (lines 28 and 49) do not capture or check the HTTP response. If the server returns an error status (4xx/5xx), the tests may still pass if the error page has a matching title or DOM structure. Capture the response and assert it's OK before checking page content.🛡️ Proposed fix
// The main UI is served at '/' (or '/sliders') from compiled-in PAGE_index - await page.goto('/'); + const response = await page.goto('/'); + expect(response?.ok(), 'Main UI should return HTTP 2xx/3xx').toBeTruthy(); // Wait for page to be loaded (don't wait for networkidle as API calls may hang) await page.waitForLoadState('load');test('should have basic UI elements', async ({ page }) => { - await page.goto('/'); + const response = await page.goto('/'); + expect(response?.ok(), 'Main UI should return HTTP 2xx/3xx').toBeTruthy(); await page.waitForLoadState('load');Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` at line 28, The page navigation calls using page.goto('/') do not check the HTTP response; update both uses of page.goto (the initial navigation and the later navigation) to capture the response (assign the return value) and assert the response is successful (e.g., response.ok() or status < 400) before proceeding to title/DOM assertions so tests fail on server errors rather than on matching error pages.
🧹 Nitpick comments (3)
.github/workflows/qemu-e2e-test.yml (1)
93-97: ⚡ Quick winAdd
@playwright/testtopackage.jsoninstead of installing it inline.Running
npm install@playwright/test`` afternpm cimodifies `node_modules` beyond the locked state, which can cause version drift and makes the Playwright dependency implicit rather than tracked in version control.Suggested approach
Add to
package.jsondevDependencies:"devDependencies": { "`@playwright/test`": "^1.x" }Then simplify the step:
- name: Install Playwright Browsers - run: | - npm install `@playwright/test` - npx playwright install --with-deps chromium + run: npx playwright install --with-deps chromium🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qemu-e2e-test.yml around lines 93 - 97, Add `@playwright/test` to package.json devDependencies (e.g., under "devDependencies": { "`@playwright/test`": "^1.x" }) instead of running `npm install `@playwright/test`` in the CI step; update the GitHub Actions step named "Install Playwright Browsers" to remove the inline `npm install `@playwright/test`` call and keep only `npx playwright install --with-deps chromium` after `npm ci`, ensuring Playwright is installed from the lockfile and the dependency is tracked in version control.e2e-tests/index.spec.js (1)
34-34: ⚡ Quick winReplace fixed 3-second waits with condition-based waits.
Fixed timeouts (lines 34, 51) are brittle: they may be too short on slow CI runners or unnecessarily long on fast machines. Instead, wait for a specific DOM element or network condition that signals the UI is ready (e.g.,
await page.locator('#picker').waitFor()orawait page.waitForSelector('#sliders')).♻️ Suggested refactor
// Wait for page to be loaded (don't wait for networkidle as API calls may hang) await page.waitForLoadState('load'); - // Wait a bit for initial JavaScript to execute - await page.waitForTimeout(3000); + // Wait for the picker element to be attached (signals JS initialized the UI) + await page.locator('`#picker`').waitFor({ state: 'attached', timeout: 5000 }); // Check that the page title is set await expect(page).toHaveTitle(/WLED/);test('should have basic UI elements', async ({ page }) => { const response = await page.goto('/'); expect(response?.ok(), 'Main UI should return HTTP 2xx/3xx').toBeTruthy(); await page.waitForLoadState('load'); - await page.waitForTimeout(3000); + await page.locator('`#picker`').waitFor({ state: 'attached', timeout: 5000 }); // Check for the picker container (color wheel) const pickerContainer = page.locator('`#picker`');Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` at line 34, The test uses fixed sleeps via page.waitForTimeout(3000) which is brittle; replace those calls in e2e-tests/index.spec.js with condition-based waits such as awaiting a specific locator or selector that indicates readiness (e.g., use page.locator('`#picker`').waitFor() or page.waitForSelector('`#sliders`') where the UI element appears) so that the tests proceed as soon as the DOM is ready instead of a fixed 3s delay; update both occurrences that currently call page.waitForTimeout to wait on the appropriate element(s) used by the test flow.e2e-tests/fixtures.js (1)
40-41: 💤 Low valueConsider using async file write to avoid blocking the teardown phase.
fs.writeFileSyncblocks the event loop during test teardown. While acceptable for small log files, switching tofs.promises.writeFile(orfs.writeFilewith promisification) would be more consistent with the async fixture pattern and avoid potential blocking if logs grow large.♻️ Optional async alternative
+const fs = require('fs').promises; +const fsSync = require('fs'); // Ensure the console-logs directory exists -if (!fs.existsSync(CONSOLE_LOG_DIR)) { - fs.mkdirSync(CONSOLE_LOG_DIR, { recursive: true }); +if (!fsSync.existsSync(CONSOLE_LOG_DIR)) { + fsSync.mkdirSync(CONSOLE_LOG_DIR, { recursive: true }); } // ... in the fixture teardown: const logFile = path.join(CONSOLE_LOG_DIR, `${safeName}.log`); const logContent = consoleLogs.join('\n') + '\n'; - fs.writeFileSync(logFile, logContent); + await fs.writeFile(logFile, logContent);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/fixtures.js` around lines 40 - 41, The teardown is using the blocking fs.writeFileSync to write logs (logContent built from consoleLogs and written to logFile); change this to the async API by replacing fs.writeFileSync(logFile, logContent) with an await to fs.promises.writeFile(logFile, logContent) (or use fs.writeFile with a Promise) and ensure the surrounding function/fixture is declared async so the await works; keep the same logContent construction and error handling behavior but use the non-blocking write to avoid blocking the event loop during teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qemu-e2e-test.yml:
- Around line 319-325: The upload step named "Upload JS console logs" currently
fails when the console-logs/ directory is absent; update that
actions/upload-artifact@v4 step to include the input if-no-files-found: ignore
so the artifact upload is skipped instead of erroring when the directory does
not exist, keeping the rest of the step (name: js-console-logs, path:
console-logs/, retention-days: 7) unchanged.
---
Outside diff comments:
In `@e2e-tests/fixtures.js`:
- Around line 1-53: Add AI attribution comment markers around the generated
fixture block so it's clearly documented as AI-generated; specifically insert a
leading comment like "// AI: below section was generated by an AI" immediately
before the exports.test = base.test.extend({ ... }) block and a trailing comment
like "// AI: end" immediately after the end of that block (after the closing
});). Ensure the markers surround the entire console-capturing logic that
references CONSOLE_LOG_DIR, the page fixture listener setup (page.on('console',
...), page.on('pageerror', ...)), and the testInfo.attach/writeFileSync cleanup
so the AI origin is obvious while leaving exports.expect and other code
unchanged.
In `@e2e-tests/index.spec.js`:
- Around line 1-80: Add AI attribution comments surrounding the generated test
suite: insert a leading comment "// AI: below section was generated by an AI"
immediately before the block that begins with test.describe('WLED Index Page',
...) and a trailing comment "// AI: end" immediately after its closing "});" so
the entire suite (all tests within test.describe) is clearly marked as
AI-generated; ensure the comments are plain line comments at the top and bottom
of the described block so tooling and reviewers can identify the generated
section.
---
Duplicate comments:
In `@e2e-tests/index.spec.js`:
- Line 28: The page navigation calls using page.goto('/') do not check the HTTP
response; update both uses of page.goto (the initial navigation and the later
navigation) to capture the response (assign the return value) and assert the
response is successful (e.g., response.ok() or status < 400) before proceeding
to title/DOM assertions so tests fail on server errors rather than on matching
error pages.
In `@package.json`:
- Line 13: The project uses `@playwright/test` (imported by playwright.config.js,
e2e-tests/fixtures.js and e2e-tests/index.spec.js) but package.json only defines
the "test:e2e" script calling npx; add "`@playwright/test`" to package.json
devDependencies so the package is installed in node_modules instead of fetched
on-demand, then run npm install to update package-lock.json (or yarn.lock) so CI
and local installs are deterministic.
---
Nitpick comments:
In @.github/workflows/qemu-e2e-test.yml:
- Around line 93-97: Add `@playwright/test` to package.json devDependencies (e.g.,
under "devDependencies": { "`@playwright/test`": "^1.x" }) instead of running `npm
install `@playwright/test`` in the CI step; update the GitHub Actions step named
"Install Playwright Browsers" to remove the inline `npm install
`@playwright/test`` call and keep only `npx playwright install --with-deps
chromium` after `npm ci`, ensuring Playwright is installed from the lockfile and
the dependency is tracked in version control.
In `@e2e-tests/fixtures.js`:
- Around line 40-41: The teardown is using the blocking fs.writeFileSync to
write logs (logContent built from consoleLogs and written to logFile); change
this to the async API by replacing fs.writeFileSync(logFile, logContent) with an
await to fs.promises.writeFile(logFile, logContent) (or use fs.writeFile with a
Promise) and ensure the surrounding function/fixture is declared async so the
await works; keep the same logContent construction and error handling behavior
but use the non-blocking write to avoid blocking the event loop during teardown.
In `@e2e-tests/index.spec.js`:
- Line 34: The test uses fixed sleeps via page.waitForTimeout(3000) which is
brittle; replace those calls in e2e-tests/index.spec.js with condition-based
waits such as awaiting a specific locator or selector that indicates readiness
(e.g., use page.locator('`#picker`').waitFor() or page.waitForSelector('`#sliders`')
where the UI element appears) so that the tests proceed as soon as the DOM is
ready instead of a fixed 3s delay; update both occurrences that currently call
page.waitForTimeout to wait on the appropriate element(s) used by the test flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3855e87f-4376-4b4e-99a1-d1a0c32366ca
📒 Files selected for processing (16)
.github/workflows/qemu-e2e-test.yml.gitignoree2e-tests/IMPLEMENTATION_SUMMARY.mde2e-tests/README.mde2e-tests/fixtures.jse2e-tests/index.spec.jse2e-tests/other-pages.spec.jse2e-tests/settings.spec.jslib/openeth_mac/esp_eth_mac_openeth.clib/openeth_mac/library.jsonlib/openeth_mac/openeth.hpackage.jsonplatformio.inisdkconfig.defaults.qemuwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (3)
- e2e-tests/IMPLEMENTATION_SUMMARY.md
- wled00/wled.h
- wled00/wled.cpp
✅ Files skipped from review due to trivial changes (3)
- lib/openeth_mac/library.json
- e2e-tests/README.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e-tests/settings.spec.js
- e2e-tests/other-pages.spec.js
👮 Files not reviewed due to content moderation or server errors (4)
- lib/openeth_mac/openeth.h
- lib/openeth_mac/esp_eth_mac_openeth.c
- platformio.ini
- sdkconfig.defaults.qemu
| - name: Upload JS console logs | ||
| uses: actions/upload-artifact@v4 | ||
| if: always() | ||
| with: | ||
| name: js-console-logs | ||
| path: console-logs/ | ||
| retention-days: 7 |
There was a problem hiding this comment.
Add if-no-files-found: ignore to prevent upload failure when directory is missing.
The console-logs/ directory is only created by Playwright fixtures when tests actually run. If the workflow fails before Playwright executes any tests, this directory won't exist and the upload step will fail unnecessarily.
Suggested fix
- name: Upload JS console logs
uses: actions/upload-artifact@v4
if: always()
with:
name: js-console-logs
path: console-logs/
retention-days: 7
+ if-no-files-found: ignore🧰 Tools
🪛 zizmor (1.25.2)
[error] 320-320: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/qemu-e2e-test.yml around lines 319 - 325, The upload step
named "Upload JS console logs" currently fails when the console-logs/ directory
is absent; update that actions/upload-artifact@v4 step to include the input
if-no-files-found: ignore so the artifact upload is skipped instead of erroring
when the directory does not exist, keeping the rest of the step (name:
js-console-logs, path: console-logs/, retention-days: 7) unchanged.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
package.json (1)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare Playwright in
devDependenciesinstead of relying on transientnpxresolution.Line 13 runs Playwright via
npx, but the package manifest still doesn’t declare@playwright/test; installs can differ between environments.Suggested fix
"scripts": { "build": "node tools/cdata.js", "test": "node --test", - "test:e2e": "npx playwright test", + "test:e2e": "playwright test", "dev": "nodemon -e js,html,htm,css,png,jpg,gif,ico,js -w tools/ -w wled00/data/ -x node tools/cdata.js" }, + "devDependencies": { + "`@playwright/test`": "^1.54.0" + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 13, The e2e script "test:e2e" calls Playwright via npx but `@playwright/test` is not declared; add `@playwright/test` to devDependencies (e.g., install with npm/yarn/pnpm: npm install --save-dev `@playwright/test`@latest) or add the "`@playwright/test`": "<version>" entry to package.json devDependencies and run install so the "test:e2e" script resolves reliably across environments.e2e-tests/index.spec.js (1)
28-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert
page.goto()response success before title/DOM checks.Line 28 and Line 49 navigate but never verify the HTTP result; tests can continue on unexpected responses.
Suggested fix
- await page.goto('/'); + const response = await page.goto('/'); + expect(response?.ok(), 'GET / should return HTTP 2xx/3xx').toBeTruthy(); @@ - await page.goto('/'); + const response = await page.goto('/'); + expect(response?.ok(), 'GET / should return HTTP 2xx/3xx').toBeTruthy();Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` around lines 28 - 29, When calling page.goto('/') in the tests (the page.goto calls in the spec), capture the returned Response and assert it indicates a successful navigation before performing title/DOM checks; for example, store the return value from page.goto('/') (the Response object), assert response is not null and response.ok() (or response.status() === 200) and fail the test if the navigation failed, then proceed with title/DOM assertions—update both occurrences (the page.goto calls) to perform this response validation.
🧹 Nitpick comments (1)
e2e-tests/index.spec.js (1)
34-35: ⚡ Quick winReplace fixed sleeps with condition-based waits to reduce flakiness.
Line 34 and Line 51 use hard-coded 3s delays; this slows fast runs and flakes on slow ones. Prefer waiting on a deterministic UI condition.
Suggested fix
- await page.waitForTimeout(3000); + await expect(page.locator('`#picker`')).toBeAttached({ timeout: 10000 }); @@ - await page.waitForTimeout(3000); + await expect(page.locator('`#picker`')).toBeAttached({ timeout: 10000 });Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` around lines 34 - 35, Replace the fixed 3s sleeps (calls to page.waitForTimeout(3000)) with deterministic condition-based waits: identify the UI condition you actually need (e.g., element visibility, text content, or a network response) and use Playwright/Puppeteer wait helpers such as page.waitForSelector, page.waitForFunction or page.waitForResponse instead of page.waitForTimeout; update both occurrences of page.waitForTimeout(3000) in the e2e-tests/index.spec.js test so each waits on the specific selector/text/response that signals the app is ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e-tests/fixtures.js`:
- Around line 36-41: The current log filename generation using safeName from
testInfo.titlePath can be overwritten across retries or parallel projects;
update the filename logic used for logFile so it incorporates unique run
identifiers (e.g. include testInfo.retry, testInfo.project?.name or
testInfo.project.name, and a per-worker or timestamp like testInfo.workerIndex
or Date.now()) in addition to safeName to guarantee uniqueness; modify the
creation of safeName/logFile (references: safeName, testInfo.titlePath, logFile,
CONSOLE_LOG_DIR) to append those identifiers before writing with
fs.writeFileSync.
---
Duplicate comments:
In `@e2e-tests/index.spec.js`:
- Around line 28-29: When calling page.goto('/') in the tests (the page.goto
calls in the spec), capture the returned Response and assert it indicates a
successful navigation before performing title/DOM checks; for example, store the
return value from page.goto('/') (the Response object), assert response is not
null and response.ok() (or response.status() === 200) and fail the test if the
navigation failed, then proceed with title/DOM assertions—update both
occurrences (the page.goto calls) to perform this response validation.
In `@package.json`:
- Line 13: The e2e script "test:e2e" calls Playwright via npx but
`@playwright/test` is not declared; add `@playwright/test` to devDependencies (e.g.,
install with npm/yarn/pnpm: npm install --save-dev `@playwright/test`@latest) or
add the "`@playwright/test`": "<version>" entry to package.json devDependencies
and run install so the "test:e2e" script resolves reliably across environments.
---
Nitpick comments:
In `@e2e-tests/index.spec.js`:
- Around line 34-35: Replace the fixed 3s sleeps (calls to
page.waitForTimeout(3000)) with deterministic condition-based waits: identify
the UI condition you actually need (e.g., element visibility, text content, or a
network response) and use Playwright/Puppeteer wait helpers such as
page.waitForSelector, page.waitForFunction or page.waitForResponse instead of
page.waitForTimeout; update both occurrences of page.waitForTimeout(3000) in the
e2e-tests/index.spec.js test so each waits on the specific
selector/text/response that signals the app is ready.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebfc2a52-612a-4639-b4f1-b20944feb5fe
📒 Files selected for processing (16)
.github/workflows/qemu-e2e-test.yml.gitignoree2e-tests/IMPLEMENTATION_SUMMARY.mde2e-tests/README.mde2e-tests/fixtures.jse2e-tests/index.spec.jse2e-tests/other-pages.spec.jse2e-tests/settings.spec.jslib/openeth_mac/esp_eth_mac_openeth.clib/openeth_mac/library.jsonlib/openeth_mac/openeth.hpackage.jsonplatformio.inisdkconfig.defaults.qemuwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (3)
- e2e-tests/IMPLEMENTATION_SUMMARY.md
- wled00/wled.h
- wled00/wled.cpp
✅ Files skipped from review due to trivial changes (3)
- lib/openeth_mac/library.json
- e2e-tests/README.md
- sdkconfig.defaults.qemu
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- e2e-tests/settings.spec.js
- e2e-tests/other-pages.spec.js
- .github/workflows/qemu-e2e-test.yml
make sure that logfile is unique Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
e2e-tests/index.spec.js (1)
30-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNavigation response not validated (persists from previous review).
Both
page.goto('/sliders')calls (lines 30 and 50) ignore the HTTP response, so a 404 or 5xx could still pass if the title/DOM assertions happen to match. The JSON API tests (lines 60, 70) demonstrate the correct pattern withpage.request.get()and explicitresponse.status()checks.🔍 Proposed fix
- await page.goto('/sliders'); + const response = await page.goto('/sliders'); + expect(response?.ok(), '/sliders should return HTTP 2xx/3xx').toBeTruthy(); await page.waitForLoadState('load');Apply the same pattern at line 50.
Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/index.spec.js` at line 30, The page navigation calls using page.goto('/sliders') do not validate the HTTP response, so capture the navigation response (e.g., const response = await page.goto('/sliders')) and assert its status is OK (use response.status() === 200 or check 2xx range) before proceeding with DOM/title assertions; apply this change to both occurrences of page.goto('/sliders') (and mirror the pattern used by page.request.get() / response.status() in the JSON API tests) so a 404/5xx will fail the test early.e2e-tests/fixtures.js (1)
36-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilename collision risk remains unresolved.
Lines 40-41 contain the fix for the filename uniqueness issue raised in the previous review, but it's commented out with the note "not (yet) compatible with artefacts collection step." While the developer is aware of the collision risk (parallel runs or retries can overwrite the same
.logfile), deferring the fix means diagnostics may still be lost in multi-worker or retry scenarios.Run the following script to verify whether the CI artifact collection step can handle unique suffixes in filenames:
#!/bin/bash # Description: Check CI artifact upload patterns to understand filename constraints # Search for artifact upload steps and path patterns rg -A5 -B2 'upload-artifact|console-logs' .github/workflows/qemu-e2e-test.yml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/fixtures.js` around lines 36 - 43, The current log file naming uses safeName and can collide across workers/retries; un-comment and restore a uniqueSuffix (or alternatively append a timestamp/UUID) when building logFile so filenames include testInfo.project.name, testInfo.workerIndex and testInfo.retry (the uniqueSuffix used in the diff) to ensure uniqueness; update the creation of logFile (the variable referenced where fs.writeFileSync writes to CONSOLE_LOG_DIR) to use the unique filename and ensure the suffix is sanitized the same way as safeName so CI artifact collection patterns still match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qemu-e2e-test.yml:
- Around line 93-96: The workflow's "Install Playwright Browsers" step
redundantly runs "npm install `@playwright/test`" after "npm ci", which can
override locked versions; remove the explicit install call and keep only the
browser installation command (npx playwright install --with-deps chromium), or
if `@playwright/test` is not in package.json, add it to package.json and rely on
npm ci to install the locked version instead; update the step named "Install
Playwright Browsers" and ensure only the browser installation (npx playwright
install --with-deps chromium) runs in that step.
---
Duplicate comments:
In `@e2e-tests/fixtures.js`:
- Around line 36-43: The current log file naming uses safeName and can collide
across workers/retries; un-comment and restore a uniqueSuffix (or alternatively
append a timestamp/UUID) when building logFile so filenames include
testInfo.project.name, testInfo.workerIndex and testInfo.retry (the uniqueSuffix
used in the diff) to ensure uniqueness; update the creation of logFile (the
variable referenced where fs.writeFileSync writes to CONSOLE_LOG_DIR) to use the
unique filename and ensure the suffix is sanitized the same way as safeName so
CI artifact collection patterns still match.
In `@e2e-tests/index.spec.js`:
- Line 30: The page navigation calls using page.goto('/sliders') do not validate
the HTTP response, so capture the navigation response (e.g., const response =
await page.goto('/sliders')) and assert its status is OK (use response.status()
=== 200 or check 2xx range) before proceeding with DOM/title assertions; apply
this change to both occurrences of page.goto('/sliders') (and mirror the pattern
used by page.request.get() / response.status() in the JSON API tests) so a
404/5xx will fail the test early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8474f770-bded-4256-a61a-9307421d7116
📒 Files selected for processing (17)
.github/workflows/qemu-e2e-test.yml.gitignoree2e-tests/IMPLEMENTATION_SUMMARY.mde2e-tests/QEMU-ISSUES.mde2e-tests/README.mde2e-tests/fixtures.jse2e-tests/index.spec.jse2e-tests/other-pages.spec.jse2e-tests/settings.spec.jslib/openeth_mac/esp_eth_mac_openeth.clib/openeth_mac/library.jsonlib/openeth_mac/openeth.hpackage.jsonplatformio.inisdkconfig.defaults.qemuwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (4)
- e2e-tests/IMPLEMENTATION_SUMMARY.md
- wled00/wled.h
- e2e-tests/QEMU-ISSUES.md
- wled00/wled.cpp
✅ Files skipped from review due to trivial changes (3)
- lib/openeth_mac/library.json
- .gitignore
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e-tests/README.md
- e2e-tests/settings.spec.js
- e2e-tests/other-pages.spec.js
- platformio.ini
| - name: Install Playwright Browsers | ||
| run: | | ||
| npm install @playwright/test | ||
| npx playwright install --with-deps chromium |
There was a problem hiding this comment.
Redundant npm install may override locked dependency version.
npm ci (line 91) already installs dependencies from package-lock.json. Running npm install @playwright/test`` without a version specifier can install the latest version, potentially overriding the locked version and causing inconsistent test behavior.
If @playwright/test is already in package.json, remove the redundant install:
Suggested fix
- name: Install Playwright Browsers
- run: |
- npm install `@playwright/test`
- npx playwright install --with-deps chromium
+ run: npx playwright install --with-deps chromium🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/qemu-e2e-test.yml around lines 93 - 96, The workflow's
"Install Playwright Browsers" step redundantly runs "npm install
`@playwright/test`" after "npm ci", which can override locked versions; remove the
explicit install call and keep only the browser installation command (npx
playwright install --with-deps chromium), or if `@playwright/test` is not in
package.json, add it to package.json and rely on npm ci to install the locked
version instead; update the step named "Install Playwright Browsers" and ensure
only the browser installation (npx playwright install --with-deps chromium) runs
in that step.
Adds a complete CI workflow that builds WLED-MM firmware, runs it in QEMU ESP32 emulation, and validates the web UI using Playwright browser tests.
What this PR does
.github/workflows/qemu-e2e-test.yml): Builds firmware with OpenETH networking, launches QEMU with port forwarding, verifies HTTP connectivity, then runs Playwright testslib/openeth_mac/): ESP-IDF v4.4.7 source foresp_eth_mac_new_openeth()since the pre-built Tasmotalibethernet.adoesn't include ite2e-tests/): Tests all main UI pages using correct WLED server routes (/,/settings,/settings/wifi,/liveview, etc.) and validates JSON API endpointse2e-tests/fixtures.js): Shared Playwright fixture captures all browser console messages (log, warn, error, info, debug) and page errors per test, writes per-test log files toconsole-logs/, attaches them to the HTML report, and uploads as a separatejs-console-logsCI artifactServer Routes Tested
The tests use WLED's compiled-in page routes (not filesystem paths):
/— Main UI (index page)/settings,/settings/wifi,/settings/leds,/settings/ui,/settings/sync,/settings/time,/settings/sec,/settings/um,/settings/2D— Settings pages/simple.htm,/welcome,/update,/liveview— Other pages/json/info,/json/state— JSON API endpointsBuild Environment
esp32_16MB_QEMU_debug(ethernet + OpenETH + WLED_QEMU flag)@playwright/testinstalled in CI before browser downloadOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores