ci: adds Playwright tests - #146
Open
bmuenzenmeyer wants to merge 5 commits into
Open
bmuenzenmeyer wants to merge 5 commits into
bmuenzenmeyer wants to merge 5 commits into
Conversation
#143 moved the scheme into `baseURL` as a hard-coded `https://`, so local builds now point their assets at https://localhost:3000, which `serve` doesn't answer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t
Since the doc-kit 2 migration (#138) the navbar rendered the ui-components ThemeToggle directly. Only islands hydrate, so the button never opened. Use doc-kit's ThemeToggle island instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t
Ports nodejs.org's playwright.yml and adds tests for asset URLs, the sitemap, navigation, search and the theme toggle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/web-infra @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved CI permissions and telemetry handling issues, plus a sitemap origin validation gap, block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Adds Playwright end-to-end coverage for the Learn site, including local serving and Vercel preview CI.
Changes:
- Adds sitemap, asset, navigation, theme, search, and 404 tests.
- Configures Playwright, dependencies, CI, documentation, and artifact handling.
- Updates deployment-aware URLs and the hydrated theme toggle.
| File | Reviewed change | Findings |
|---|---|---|
tests/e2e/sitemap.spec.mjs |
Sitemap and search-index checks | Moderate (1 vote): validate URLs against the expected base origin. |
tests/e2e/interactions.spec.mjs |
Navigation and interaction tests | — |
tests/e2e/helpers.mjs |
Shared E2E utilities | Critical (2 votes): exclude or separately allowlist /learn/_vercel/ telemetry URLs. |
tests/e2e/assets.spec.mjs |
Asset validation | Critical (2 votes): account for intentional /learn/_vercel/ telemetry scripts. |
playwright.config.mjs |
Playwright and server configuration | — |
package.json |
Test scripts and dependencies | — |
package-lock.json |
Dependency lock updates | — |
eslint.config.mjs |
E2E lint configuration | — |
doc-kit.config.mjs |
Environment-specific origins | — |
CONTRIBUTING.md |
E2E testing documentation | — |
components/Navigation/index.jsx |
Hydrated theme toggle | — |
.gitignore |
Playwright artifact exclusions | — |
.github/workflows/playwright.yml |
Vercel preview CI workflow | Critical (1 vote): grant pull-requests: read and deployments: read permissions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+27
| permissions: | ||
| contents: read | ||
| actions: read |
| for (const url of assetURLs) { | ||
| const { origin: assetOrigin, pathname } = new URL(url); | ||
| expect.soft(assetOrigin, url).toBe(origin); | ||
| expect.soft(pathname, url).toMatch(/^\/learn\/assets\//); |
|
|
||
| const isOurs = url => { | ||
| const { origin: o, pathname } = new URL(url); | ||
| return o === origin && pathname.startsWith('/learn/'); |
avivkeller
approved these changes
Sep 23, 2026
On small screens the sidebar collapses into a dropdown, which needs JavaScript to open. Since the doc-kit 2 migration (#138) only islands hydrate, so the dropdown rendered but did nothing. Register the sidebar as an island, like Authors, and cover small screens in the e2e tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AkQ47hh78xyt4wWG9GVL5t
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After the incident today (thanks @jonasgeiler by the way!) , I wanted to shore up this repo (along with the other PRs #144 #145). Playwright would have likely caught these errors (in addition to use simply looking at the preview)
You'll notice in the log below too that the coding agent noticed two other bugs. One still in production!
Edit: manual review also found the same islands problem on small viewports that led to no nav. Fixed here too.
The Playwright workflow and 13 starter tests are done and pass against a local build. I put back the
https://httpsbug from #133, rebuilt, and 8 of the 13 tests failed, so they catch it. The patch holds 3 commits on top ofmain(6976605); apply it withgit am learn-playwright.patch. The workflow itself hasn't run in GitHub Actions yet.Writing the tests turned up two more bugs on
main, so two of the commits are fixes. You can split those out if you'd rather send them separately.https://prefix fromoriginto fix production build #143, a local build points its CSS and JS athttps://localhost:3000, which the local server doesn't answer. The fix keepshttp/httpsas part oforigin, so production, preview and local each get the right one.playwright.yml): ported from nodejs.org. It waits for the Vercel preview and runs Chromium against it. One change: it doesn't skip Dependabot PRs, since Migrate to new doc-kit packages #138 was a Dependabot bump. zizmor reports no problems with it./learn/assets/, the site font must be applied, and nothing under/learn/may fail to load.npm run serveandnpm run test:e2escripts, a Playwright config that starts the local server itself, linting set up for the test files, and a "Running the end-to-end tests" section in CONTRIBUTING.md.Made with help from a frontier model. The core repo suggests we remove the commit data - but I am retaining it for now as it was truly helpful.