feat: add minimal Playwright smoke test#534
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds Playwright-based end-to-end browser testing infrastructure to the project. It includes configuration files, test scripts, CI integration, documentation, and a smoke test that validates SDK functionality in actual browser environments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
playwright.config.ts (1)
14-18: Firefox and WebKit projects are defined but never exercised in CI.CI runs
playwright test --project=chromiumonly. The extra project definitions are fine for local use, but if there's no intention to run them in CI, consider adding a comment to clarify — or remove them to keep the config minimal and aligned with the "minimal smoke test" goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 14 - 18, The projects array in playwright.config.ts declares "firefox" and "webkit" projects (names: "firefox", "webkit") but CI only runs --project=chromium; either remove these extra project entries to keep the config minimal for the CI smoke test, or add a one-line comment above projects explaining that firefox and webkit are kept for local testing only and intentionally not executed in CI; update the projects block accordingly (or add the clarifying comment) so intent is explicit.package.json (1)
54-55: Minor glob inconsistency informatvsformat:fix.
formatuses'examples/**/*.(js|jsx)'butformat:fixuses'examples/**/*.(js|jsx|ts)'— the|tsextension is only informat:fix. This appears to be a pre-existing inconsistency but worth noting since these lines were touched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 54 - 55, The npm scripts "format" and "format:fix" have inconsistent example file globs: "format" uses 'examples/**/*.(js|jsx)' while "format:fix" uses 'examples/**/*.(js|jsx|ts)'; update the "format" script to include TypeScript ('examples/**/*.(js|jsx|ts)') so both "format" and "format:fix" use the same glob pattern, ensuring consistency between the two scripts..github/workflows/ci.yml (1)
20-25: Double build in CI.
yarn buildruns at Line 20, and then the PlaywrightwebServer.command(yarn build && serve . -p 3000) triggers a second build. Consider splitting the webServer command so CI can skip the redundant build — e.g., useserve . -p 3000as the webServer command and add a separate prerequisite or guard:command: process.env.CI ? "serve . -p 3000" : "yarn build && serve . -p 3000",This would cut CI time, especially as the project grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 20 - 25, CI runs `yarn build` then Playwright's webServer.command rebuilds the app, causing a redundant build; update the Playwright `webServer.command` to skip the build in CI by using `process.env.CI` to choose `serve . -p 3000` when CI is set and keep `yarn build && serve . -p 3000` locally, and keep the existing `yarn build` step in the CI workflow so artifacts are built before starting the server.e2e/smoke.spec.ts (1)
7-15: Prefer locator-based assertions overwaitForSelector.Modern Playwright recommends the Locator API with web-first assertions. This gives better error messages and auto-retry semantics:
♻️ Suggested refactor
test("bundled SDK loads and runs in browser", async ({ page }) => { await page.goto("/e2e/fixtures/smoke.html"); - await page.waitForSelector("#status:has-text('ok')", { timeout: 10000 }); + await expect(page.locator("#status")).toHaveText("ok", { timeout: 10_000 }); const result = await page.evaluate(() => { return (window as { __sdkReady?: boolean }).__sdkReady; }); expect(result).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/smoke.spec.ts` around lines 7 - 15, The test "bundled SDK loads and runs in browser" currently uses page.waitForSelector; replace that with a locator-based assertion using Playwright's Locator API and web-first assertions: locate the element (e.g., page.locator('#status')) and assert its text content with expect(...).toHaveText('ok', { timeout: 10000 }) so you get auto-retry and better errors; update the block around the page.waitForSelector call in the test function to use the locator+expect pattern before evaluating window.__sdkReady.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Around line 19-24: The webServer command uses the wrong flag for the Vercel
serve binary so it won't bind to the intended port; update the webServer.command
string in the Playwright config (the webServer object) to use the listen flag
(-l or --listen) instead of -p, e.g., replace "yarn build && serve . -p 3000"
with "yarn build && serve . -l 3000" so the server binds to port 3000 and tests
can start.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 20-25: CI runs `yarn build` then Playwright's webServer.command
rebuilds the app, causing a redundant build; update the Playwright
`webServer.command` to skip the build in CI by using `process.env.CI` to choose
`serve . -p 3000` when CI is set and keep `yarn build && serve . -p 3000`
locally, and keep the existing `yarn build` step in the CI workflow so artifacts
are built before starting the server.
In `@e2e/smoke.spec.ts`:
- Around line 7-15: The test "bundled SDK loads and runs in browser" currently
uses page.waitForSelector; replace that with a locator-based assertion using
Playwright's Locator API and web-first assertions: locate the element (e.g.,
page.locator('#status')) and assert its text content with
expect(...).toHaveText('ok', { timeout: 10000 }) so you get auto-retry and
better errors; update the block around the page.waitForSelector call in the test
function to use the locator+expect pattern before evaluating window.__sdkReady.
In `@package.json`:
- Around line 54-55: The npm scripts "format" and "format:fix" have inconsistent
example file globs: "format" uses 'examples/**/*.(js|jsx)' while "format:fix"
uses 'examples/**/*.(js|jsx|ts)'; update the "format" script to include
TypeScript ('examples/**/*.(js|jsx|ts)') so both "format" and "format:fix" use
the same glob pattern, ensuring consistency between the two scripts.
In `@playwright.config.ts`:
- Around line 14-18: The projects array in playwright.config.ts declares
"firefox" and "webkit" projects (names: "firefox", "webkit") but CI only runs
--project=chromium; either remove these extra project entries to keep the config
minimal for the CI smoke test, or add a one-line comment above projects
explaining that firefox and webkit are kept for local testing only and
intentionally not executed in CI; update the projects block accordingly (or add
the clarifying comment) so intent is explicit.
DSanich
force-pushed
the
feat/e2e-minimal
branch
from
bad1b1b to
cdd1d1a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
package.json (1)
91-92: Minor:servebreaks alphabetical order ofdevDependencies.
"serve"(line 91) appears before"qrcode-terminal"(line 92). Most tooling and conventions expect devDependencies sorted alphabetically. Not blocking, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 91 - 92, DevDependencies are out of alphabetical order: move the "serve" entry so keys are sorted alphabetically (place "qrcode-terminal" before "serve") in package.json's devDependencies section, or run a sorting tool to ensure all keys (including "qrcode-terminal" and "serve") follow alphabetical order..github/workflows/ci.yml (1)
22-25: Double build in CI — consider optimizing.The
yarn buildon line 20 already producesdist/. ThewebServer.commandin Playwright config runsyarn build && serve ...again, rebuilding unnecessarily. You could split the webServer command to skip the build when artifacts already exist, or use a serve-only command in CI.One approach: change the webServer command to just
serve . -l 3000and document thatyarn buildmust run first (which CI already guarantees). Alternatively, keep it self-contained if you value simplicity over CI speed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 22 - 25, The CI currently builds the app twice because the workflow runs `yarn build` (producing dist/) but Playwright's webServer.command triggers `yarn build && serve ...` again; update the Playwright config's webServer.command to serve the already-built artifact (e.g., use a serve-only command like `serve . -l 3000` or a conditional that skips `yarn build` when `dist/` exists) so that CI step names like "Run E2E tests" and "Install Playwright browsers" can rely on the prior `yarn build` and avoid redundant builds.playwright.config.ts (1)
14-17: Consider removing unused browser projects.The PR objective is a minimal Chromium-only smoke test, and CI runs
--project=chromium. The Firefox and WebKit projects add no value here and will confuse contributors who runplaywright testwithout--project(triggering all three browsers, which requires installing extra browser binaries).Suggested simplification
- projects: [ - { name: "chromium", use: { ...devices["Desktop Chrome"] } }, - { name: "firefox", use: { ...devices["Desktop Firefox"] } }, - { name: "webkit", use: { ...devices["Desktop Safari"] } }, - ], + projects: [ + { name: "chromium", use: { ...devices["Desktop Chrome"] } }, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 14 - 17, Remove the unused Firefox and WebKit project entries from the projects array so only the Chromium project remains; specifically delete the objects with name: "firefox" and name: "webkit" and keep { name: "chromium", use: { ...devices["Desktop Chrome"] } } in the projects array (projects) to ensure running playwright test defaults to the single Chromium smoke test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@playwright.config.ts`:
- Around line 19-24: The webServer command in the Playwright config uses the
wrong flag for the serve package; update the command string in the webServer
config (the webServer object in playwright.config.ts) to use serve's listen flag
(either -l 3000 or --listen 3000) instead of -p 3000 so the server binds to port
3000 and tests don't time out.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 22-25: The CI currently builds the app twice because the workflow
runs `yarn build` (producing dist/) but Playwright's webServer.command triggers
`yarn build && serve ...` again; update the Playwright config's
webServer.command to serve the already-built artifact (e.g., use a serve-only
command like `serve . -l 3000` or a conditional that skips `yarn build` when
`dist/` exists) so that CI step names like "Run E2E tests" and "Install
Playwright browsers" can rely on the prior `yarn build` and avoid redundant
builds.
In `@package.json`:
- Around line 91-92: DevDependencies are out of alphabetical order: move the
"serve" entry so keys are sorted alphabetically (place "qrcode-terminal" before
"serve") in package.json's devDependencies section, or run a sorting tool to
ensure all keys (including "qrcode-terminal" and "serve") follow alphabetical
order.
In `@playwright.config.ts`:
- Around line 14-17: Remove the unused Firefox and WebKit project entries from
the projects array so only the Chromium project remains; specifically delete the
objects with name: "firefox" and name: "webkit" and keep { name: "chromium",
use: { ...devices["Desktop Chrome"] } } in the projects array (projects) to
ensure running playwright test defaults to the single Chromium smoke test.
|
@rolznz In continuation of this #533 (comment) I've created a separate PR with the minimum required test for Playwright integration. All the tests I created earlier will also be distributed across separate pull requests using Jest. |
rolznz
left a comment
rolznz
left a comment
There was a problem hiding this comment.
tACK, can you address the minor comments and I'll merge it.
Thanks for the PR!
…nto feat/e2e-minimal
|
@DSanich can you check the build error? |
|
@rolznz pushed fix for CI build 🙏 |
|
I don't understand, how is that a fix? |
|
@rolznz hello! Yarn test (Jest) tried to run e2e/smoke.spec.ts, which is written for Playwright. Jest and Playwright are different runners, so the test failed. What I did: I added testPathIgnorePatterns: ['/e2e/'] to Jest so that Jest doesn't run tests from e2e/. |
|
@DSanich ah, thanks for the explanation. We might need to change the folder then since in your other PR it uses jest for e2e. https://github.com/getAlby/js-sdk/pull/535/changes Can you move the browser tests into /e2e/browser and only ignore that folder in jest? then I think we won't have issues. What do you think? |
|
@rolznz Yes, that's a great idea. I'm working on that. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/browser/smoke.spec.ts (1)
9-9: Prefer web-first assertion overwaitForSelectorfor the status check.
page.waitForSelector()is now discouraged in favour of Playwright's Locator API and built-in auto-waiting. Using Locator objects and assertions makes the code "wait-for-selector-free." The DOM-ready check can be replaced with a single web-first assertion that auto-waits and retries:♻️ Proposed refactor
- await page.waitForSelector("#status:has-text('ok')", { timeout: 10000 }); + await expect(page.locator("#status")).toContainText("ok", { timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/browser/smoke.spec.ts` at line 9, Replace the page.waitForSelector call with a web-first Locator assertion: create a Locator for "#status" (e.g., status = page.locator('#status')) and await a Playwright expect assertion such as expect(status).toHaveText('ok', { timeout: 10000 }) or expect(status).toContainText('ok', { timeout: 10000 }) so the test uses the Locator API's auto-waiting instead of waitForSelector.playwright.config.ts (2)
9-9: Consider adding a terminal reporter alongside"html"for CI diagnostics.
reporter: "html"alone produces no terminal output; CI failures require downloading and opening the HTML artefact. Adding"list"(or"github"for GitHub Actions annotations) alongside keeps logs readable without extra tooling.♻️ Proposed refactor
- reporter: "html", + reporter: process.env.CI ? [["github"], ["html"]] : "html",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` at line 9, The Playwright config currently sets reporter: "html" which yields no terminal output; change the reporter property in playwright.config.ts to an array to include a terminal reporter alongside HTML (e.g., ["list", "html"] or ["github", "html"] for Actions). Update the reporter value used in the exported config object (the reporter field) to an array and, if using "html", optionally add the html options object to set outputFolder/artifact naming so CI still uploads the artifact while terminal logs remain visible.
22-22:reuseExistingServer: !CIsilently skipsyarn buildin local dev.When a process is already listening on port 3000 locally, the entire
webServer.command— includingyarn build— is bypassed, so tests silently run against a potentially stale bundle. Consider documenting this behaviour in the e2e README, or restructure the command so the build step runs unconditionally (e.g. a separateprebuildphase).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` at line 22, The current Playwright setting reuseExistingServer (in playwright.config.ts) can skip webServer.command including yarn build when a process already listens on the port; to fix, ensure the build always runs by either (a) moving the build step out of the start command into a separate prebuild script invoked unconditionally (e.g., add a "prestart:e2e" npm script that runs yarn build and call that before starting the server), or (b) make webServer.command run the build unconditionally (e.g., "yarn build && yarn start") and/or set reuseExistingServer to false in non-CI environments so tests never run against a stale bundle; update e2e README to document this behaviour and the chosen approach so developers know why build runs or why reuseExistingServer is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@playwright.config.ts`:
- Around line 19-24: The webServer command uses the wrong flag for the serve
CLI; update the command string in the webServer config (the "command" property)
to use the correct listen flag (either "-l 3000" or "--listen 3000") instead of
"-p 3000" so the local server starts on port 3000; ensure the change is applied
to the command value referenced in playwright.config.ts (webServer.command).
---
Nitpick comments:
In `@e2e/browser/smoke.spec.ts`:
- Line 9: Replace the page.waitForSelector call with a web-first Locator
assertion: create a Locator for "#status" (e.g., status =
page.locator('#status')) and await a Playwright expect assertion such as
expect(status).toHaveText('ok', { timeout: 10000 }) or
expect(status).toContainText('ok', { timeout: 10000 }) so the test uses the
Locator API's auto-waiting instead of waitForSelector.
In `@playwright.config.ts`:
- Line 9: The Playwright config currently sets reporter: "html" which yields no
terminal output; change the reporter property in playwright.config.ts to an
array to include a terminal reporter alongside HTML (e.g., ["list", "html"] or
["github", "html"] for Actions). Update the reporter value used in the exported
config object (the reporter field) to an array and, if using "html", optionally
add the html options object to set outputFolder/artifact naming so CI still
uploads the artifact while terminal logs remain visible.
- Line 22: The current Playwright setting reuseExistingServer (in
playwright.config.ts) can skip webServer.command including yarn build when a
process already listens on the port; to fix, ensure the build always runs by
either (a) moving the build step out of the start command into a separate
prebuild script invoked unconditionally (e.g., add a "prestart:e2e" npm script
that runs yarn build and call that before starting the server), or (b) make
webServer.command run the build unconditionally (e.g., "yarn build && yarn
start") and/or set reuseExistingServer to false in non-CI environments so tests
never run against a stale bundle; update e2e README to document this behaviour
and the chosen approach so developers know why build runs or why
reuseExistingServer is disabled.
2 participants
Summary
Adds a minimal Playwright smoke test to verify the bundled SDK loads and runs correctly in a browser.
Changes
/dist/esm/lnclient.js, runsSATS(10)andresolveAmount(), asserts the resultyarn test:e2e(Chromium)@playwright/test,serveWhat the smoke test validates
SATSandresolveAmountexecute without errorsHow to run
yarn test:e2e:install # one-time setup
yarn test:e2e
Summary by CodeRabbit
Tests
Chores