Skip to content
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

chore: refactor e2e tests to playwright #5080

Open
wants to merge 166 commits into
base: master
Choose a base branch
from

Conversation

mahdikhashan
Copy link
Collaborator

@mahdikhashan mahdikhashan commented Mar 1, 2024

Final Report GSoC 2024


Project

The project has been to substitute Puppeteer framework with MS Playwright in webpack-dev-server project and refactor tests afterward.

I have set up Playwright and configured it in a way to be similar to the existing code, I have extended the default test object and added a custom matcher for snapshot comparison of objects and arrays. Since previously Jest was used as Puppeteer's test runner and Playwright has its own runner, I have added Sinon.js as a mock / stubbing library.

Tests are ran over Linux, Mac and Windows using node 18, 20 and 22. The code coverage is calculated using nyc and instanbul and it's babel plugin and pushed to CodeCov.

Constraints and Future Improvement

Since by using the Playwright we could benefit from its flaky test marking mechanism, some tests are labeled as flaky which we may want to refactor them later.

Additionally, some tests were failing which I decided to do a root-cause analysis and figured out that by replacing default-gateway npm package, it could pass them successfully. (link to pr)

Thank you

I want to sincerely thank Webpack team member @evenstensberg for taking the time to review my proposal for the project in advance. Special thanks to my mentors @snitin315 and @alexander-akait which helped me when I ran into any issues or had questions.


WIP: will refactors tests one by one and rename the e2e-playwright folder to e2e after its done

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

It will introduce playwright in the project to replace the existing puppeteer end to end framework.

Breaking Changes

Additional Info

Copy link

linux-foundation-easycla bot commented Mar 1, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@mahdikhashan mahdikhashan changed the title chore: setup playwright chore: refactor e2e tests to playwright Mar 1, 2024
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Ideally, we need to rewrite existing tests on playwright + some refactor our tests, because we have a lot of code duplication

@mahdikhashan
Copy link
Collaborator Author

mahdikhashan commented Mar 4, 2024

@alexander-akait thanks for your comment, for participating in the gsoc-2024, is this PR a valid patch requirements? if not could you introduce an issue which I can start working on? I'm a master student and a junior developer. regarding the proposal, is there anything specific things to be mentioned on or do you have a template from the previous years? also, if there is a discord of slack channel which I can share the draft version of proposal and receive early feedback, I appreciate it.

@alexander-akait
Copy link
Member

/cc @evenstensberg

@evenstensberg
Copy link
Member

I think this should work for a gsoc project. (90h project)

@snitin315
Copy link
Member

I think this should work for a gsoc project. (90h project)

Yes, I agree.

WIP: will refactors tests one by one and rename the e2e-playwright folder to e2e after its done
WIP: will rename the test and clean it up after i received a feedback from maintainers
playwright.config.js Outdated Show resolved Hide resolved
package.json Outdated
@@ -41,6 +41,7 @@
"test:coverage": "npm run test:only -- --coverage",
"test:watch": "npm run test:coverage --watch",
"test": "npm run test:coverage",
"test:e2e": "npx playwright test",

Choose a reason for hiding this comment

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

Suggested change
"test:e2e": "npx playwright test",
"test:e2e": "playwright test",

const isCI = process.env.CI === "true";

/** @type { import('@playwright/test').PlaywrightTestConfig} */
module.exports = {

Choose a reason for hiding this comment

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

Ideally you use defineConfig + '// @ts-check'

testIgnore: "**/*.ignore.*",
testDir: "./test/e2e",
snapshotPathTemplate: "./test/e2e/__snapshots__/{testFilePath}/{arg}{ext}",
fullyParallel: false,

Choose a reason for hiding this comment

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

Ideally you enable fullyParallel - didn't it work with having it enabled? Might be a potential follow-up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it didn't work at first since we need to set ports, we planed to benefit from it later on.

name: "chromium",
use: {
browserName: "chromium",
launchOptions: {

Choose a reason for hiding this comment

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

Since you set ignoreHTTPSErrors in line 26, you can remove the launchOptions lock.

],
});

module.exports = { test: mergeTests(customTest) };

Choose a reason for hiding this comment

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

in theory no need to merge

Suggested change
module.exports = { test: mergeTests(customTest) };
module.exports = { test: customTest };

@mahdikhashan mahdikhashan marked this pull request as ready for review August 4, 2024 11:15
@mahdikhashan mahdikhashan self-assigned this Aug 4, 2024
@mahdikhashan mahdikhashan force-pushed the issue-2843/refactor-e2e-tests-from-puppeter-to-playwright branch from c7510cf to 3105fe8 Compare August 9, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants