Skip to content

test: setup E2E testing via Playwright and implement some tests #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pkerschbaum
Copy link
Contributor

@pkerschbaum pkerschbaum commented Apr 6, 2025

Prerequisites checklist

What is the purpose of this pull request?

Introduces End-to-End (E2E) testing using Playwright to prevent regressions and increase confidence during development and refactoring.

What changes did you make? (Give an overview)

  • Set up Playwright for E2E testing.
    • Configured GitHub Actions CI to run E2E tests.
    • Added setup based on Docker for running tests to ensure consistent screenshot generation via Playwright's toHaveScreenshot.
  • Implemented initial E2E test scenarios.
  • Also made some components here and there more accessible - which is useful for locating things in Playwright.

Is there anything you'd like reviewers to focus on?

Notes on using Docker

The new requirement, having Docker installed, is quite heavy...
Thing is that otherwise, screenshots created by screenshot assertions (toHaveScreenshot) are not consistent, leading to flaky behavior.
As noted by Playwright:

Browser rendering can vary based on the host OS, version, settings, hardware, power source (battery vs. power adapter), headless mode, and other factors. For consistent screenshots, run tests in the same environment where the baseline screenshots were generated.

Alternatives to Docker considered:

  • not using screenshot assertions at all.
    • Problematic because they are the only way to assert aesthetics, which is important for ESLint Code Explorer
    • also, they are quite useful to get "much bang for the buck" as they cover much of a web application fast, compared to "fine-grained assertions"
  • implement some workflow using GitHub Actions and Pull Request comments to create baseline screenshots only in CI (like https://medium.com/@haleywardo/streamlining-playwright-visual-regression-testing-with-github-actions-e077fd33c27c)
    • Problem is: how to develop E2E tests locally then?
    • Also, while we could drop the Docker requirement then, we would need to add instructions to install the system dependencies required for Playwright (https://playwright.dev/docs/browsers#install-system-dependencies) - seems like adding Playwright to a project adds a requirement to the development machines of contributors, one way or the other

Notes on using Playwright compared to other options

  • vitest browser mode, downsides:
    • it is experimental currently
    • lacks documentation
    • looks like it has no tools like test recorder, time travel debugging, etc.
  • Storybook Testing, downsides:
    • visual regression testing is only possible with their SaaS product "Chromatic" (https://storybook.js.org/docs/writing-tests/visual-testing)
    • I tried it out and and it just does not feel that mature and polished like Playwright (e.g. the locator API, expect APIs of Playwright are just great; Playwright produces a great report which is expecially useful in CI, ...)

Successful CI run: https://github.com/eslint/code-explorer/actions/runs/14296315788

Copy link

netlify bot commented Apr 6, 2025

Deploy Preview for eslint-code-explorer ready!

Name Link
🔨 Latest commit cd10bf7
🔍 Latest deploy log https://app.netlify.com/sites/eslint-code-explorer/deploys/67fbc5d80ab4060008ce90d5
😎 Deploy Preview https://deploy-preview-88--eslint-code-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pkerschbaum pkerschbaum marked this pull request as ready for review April 6, 2025 20:27
@nzakas
Copy link
Member

nzakas commented Apr 8, 2025

Thanks for getting this started. I don't have time to dig in right now. I'm also not super familiar with this kind of setup, so could use some advice from the rest of the team. @eslint/eslint-team

@nzakas nzakas requested review from amareshsm and Copilot April 10, 2025 15:57
Copy link

@Copilot 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.

Copilot reviewed 51 out of 52 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

src/components/ui/popover.tsx:27

  • [nitpick] Verify that the use of role 'dialog' on PopoverContent is semantically correct for a popover component; if the component is used to display menus or tips rather than modal dialogs, a different role may be more appropriate.
role="dialog"

src/components/tree-entry.tsx:109

  • Ensure that the li element is nested within a ul or ol element to maintain proper HTML semantics.
<li className="flex items-center gap-3">

@pkerschbaum pkerschbaum requested a review from nzakas April 13, 2025 14:11
@snitin315
Copy link

I'm familiar with such setup, will review this pr

@nzakas
Copy link
Member

nzakas commented Apr 18, 2025

@snitin315 friendly ping to not forget about this

Copy link

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

@pkerschbaum Are we not running Docker in CI? If that's the case, it kind of defeats the purpose of using Docker, since the CI environment would always be inconsistent.

@pkerschbaum
Copy link
Contributor Author

@pkerschbaum Are we not running Docker in CI? If that's the case, it kind of defeats the purpose of using Docker, since the CI environment would always be inconsistent.

It is always - locally and in CI - running via Docker.
The only difference is some settings related to networking.

@pkerschbaum pkerschbaum requested a review from snitin315 April 21, 2025 18:02
@nzakas
Copy link
Member

nzakas commented Apr 22, 2025

@pkerschbaum can you take a look at the merge conflicts?

@nzakas
Copy link
Member

nzakas commented Apr 22, 2025

I tried running this locally and got this error:

> [email protected] test:e2e    
> cross-env PW_TEST_CONNECT_WS_ENDPOINT=ws://127.0.0.1:3000/ playwright test      
[WebServer] Unable to find image 'mcr.microsoft.com/playwright:v1.51.1-noble' locally
[WebServer] v1.51.1-noble: 
[WebServer] Pulling from playwright      
[WebServer] 107a4fb0af38: Pulling fs layer
[WebServer] 8c401718c984: Pulling fs layer
[WebServer] 39224aba52ce: Pulling fs layer
[WebServer] 08d1cdd3adad: Pulling fs layer
[WebServer] 08d1cdd3adad: Waiting        
[WebServer] 39224aba52ce: 
[WebServer] Verifying Checksum           
[WebServer] 39224aba52ce: Download complete
[WebServer] 107a4fb0af38: 
[WebServer] Verifying Checksum           
[WebServer] 107a4fb0af38: Download complete
[WebServer] 8c401718c984: Download complete
[WebServer] 107a4fb0af38: Pull complete
[WebServer] 8c401718c984: 
[WebServer] Pull complete                
[WebServer] 39224aba52ce: 
[WebServer] Pull complete                

Error: Timed out waiting 60000ms from config.webServer.

To open last HTML report run:

  npx playwright show-report

I'm running Windows 11.

reporter: process.env.CI ? [["html"], ["github"]] : "html",

use: {
baseURL: `http://${process.env.CI ? "localhost" : "host.docker.internal"}:5173`,

Choose a reason for hiding this comment

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

We need a more specific check because env vars are always string values.

Suggested change
baseURL: `http://${process.env.CI ? "localhost" : "host.docker.internal"}:5173`,
baseURL: `http://${process.env.CI === "true" ? "localhost" : "host.docker.internal"}:5173`,

Also as per this check we are running locally on CI and docker in local dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants