Skip to content

Conversation

@avivkeller
Copy link
Member

@avivkeller avivkeller commented May 14, 2025

Description

This PR, when complete, will add Playwright E2E tests to our workflows.

  • CI
  • general-behavior.spec.ts (The general behavior, such as navigation, search, theme, language, etc)
  • What else should we test? Update this list at your whim
  • What else should we test? Update this list at your whim
  • What else should we test? Update this list at your whim
  • What else should we test? Update this list at your whim
  • What else should we test? Update this list at your whim
  • What else should we test? Update this list at your whim

Validation

Related Issues

Fixes #7395

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@vercel
Copy link

vercel bot commented May 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 23, 2025 0:05am

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟠 82 🔗
/en/about/previous-releases 🟢 98 🟢 100 🟢 100 🟠 83 🔗
/en/download 🟢 98 🟢 100 🟢 96 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@avivkeller
Copy link
Member Author

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.30%. Comparing base (c353780) to head (2e9b423).
Report is 7 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/i18n/lib/index.mjs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7749      +/-   ##
==========================================
+ Coverage   74.92%   75.30%   +0.37%     
==========================================
  Files          97       96       -1     
  Lines        7860     7847      -13     
  Branches      198      192       -6     
==========================================
+ Hits         5889     5909      +20     
+ Misses       1970     1937      -33     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avivkeller avivkeller marked this pull request as ready for review May 15, 2025 17:03
Copilot AI review requested due to automatic review settings May 15, 2025 17:03
@avivkeller avivkeller requested review from a team as code owners May 15, 2025 17:03
@avivkeller
Copy link
Member Author

Marking as ready to review as this is ready to review–although feel free to suggest the addition of more tests

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces Playwright E2E tests to verify core website functionality including theme toggling, language selection, search, and navigation responsiveness. Key changes include the addition of a comprehensive test suite in apps/site/tests/e2e/general-behavior.spec.ts, a dedicated Playwright configuration file (apps/site/playwright.config.ts) for managing test execution, and workflow integration via a new GitHub Actions workflow for Playwright testing.

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

File Description
apps/site/tests/e2e/general-behavior.spec.ts Adds E2E tests for theme, language, search, and navigation checks.
apps/site/playwright.config.ts Defines Playwright configuration for test settings and projects.
apps/site/package.json Updates scripts and dependencies to include Playwright testing.
.github/workflows/playwright.yml Introduces a GitHub Actions workflow to run Playwright tests.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)

apps/site/tests/e2e/general-behavior.spec.ts:35

  • Consider extracting the common getTheme function into a shared helper to avoid duplication across theme-related tests.
const getTheme = () => page.evaluate(() => document.documentElement.dataset.theme);

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! thanks for this! 🫶

@dario-piotrowicz
Copy link
Member

Marking as ready to review as this is ready to review–although feel free to suggest the addition of more tests

I feel like setting the thing up is good enough to merge, other tests if needed can always be incrementally added 🙂

@dario-piotrowicz
Copy link
Member

Successful Playwright Tests: https://github.com/nodejs/nodejs.org/actions/runs/15032077849/job/42246679839?pr=7749

Is the test reports uploading correctly configured? 🤔 https://github.com/nodejs/nodejs.org/actions/runs/15032077849/job/42246679839?pr=7749#step:12:12

@avivkeller
Copy link
Member Author

Is the test reports uploading correctly configured? 🤔 nodejs/nodejs.org/actions/runs/15032077849/job/42246679839?pr=7749#step:12:12

It looks like the github reporter doesn't upload any files. I'll set it to use github AND html.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe upped contributing.md

@bjohansebas
Copy link
Member

a test would be to verify that when the language is changed, the texts actually update and match what's defined in the JSON (for example, in the navigation or the footer) (#7394).

That's why the need for end-to-end testing arose.

@avivkeller
Copy link
Member Author

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😄

(as I mentioned I would not stress on having everything tested in this first PR)

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW so cool to have that thanks Aviv !

@aymen94
Copy link
Member

aymen94 commented May 19, 2025

LGTM

@avivkeller
Copy link
Member Author

@dario-piotrowicz @bmuenzenmeyer You both made comments on the use of Test IDs vs Roles / Locators. I've made some changes, could you take a look and see if it's to your liking?

Signed-off-by: Aviv Keller <[email protected]>
@dario-piotrowicz
Copy link
Member

Thanks for the ping @avivkeller 🙂

The changes still look good to me, I mentioned test-ids as my personal preference and asked if people were on board with that, @bmuenzenmeyer seems to prefer the a11y approach which is quite common and established as well, and I am totally happy with that as well (although my personal preference remains test-ids1), so I am really happy with whatever you think it's best, I don't want by any means be a blocker here 🙂

PS: one small thing, if you decide to go one way or the other I would probably try to go all-in and not use test-ids sometimes and a11y locators others (see theme vs nav as Brian mentioned).

Footnotes

  1. I find testids very explicit and robust, I think they are very clear and unambiguous, if (which I believe is a very great thing which I am totally for) a11y aspects need to be tested I would test them directly (i.e. I'd locate an element with its test-id and then validate that it has the correct a11y attributes, instead of using the a11y attributes to locate it)

@avivkeller
Copy link
Member Author

avivkeller commented May 23, 2025

@ovflowd Chromatic does support Playwright, but it takes some extra GitHub Actions logic, so I'd prefer to do it in a follow up, if you don't mind.

Once we see this working on PRs, I'll open a follow-up to integrate this with the Chromatic workflow.

@avivkeller
Copy link
Member Author

I think i've resolved all concerns, so merge when ready :-)

@AugustinMauroy AugustinMauroy added this pull request to the merge queue May 23, 2025
Merged via the queue into main with commit 3eb8f8c May 23, 2025
19 checks passed
@AugustinMauroy AugustinMauroy deleted the playwright branch May 23, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce Playwright functional tests

9 participants