Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add Playwright for end-to-end testing #76

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

Merged
haoqunjiang merged 1 commit into vuejs:main from mxschmitt:feature-add-playwright
Sep 26, 2022

Conversation

Copy link
Contributor

@mxschmitt mxschmitt commented Mar 4, 2022
edited
Loading

Hello, 👋
Playwright is getting more and more traction over the last years and we should provide users an easy integration with the Vue ecosystem so users benefit from it.
This Pull Request adds Playwright as an end-to-end testing solution like it's done for Cypress. It might make sense to throw an error if the user selects both variants for e2e testing.

We'll follow-up once the component testing story with Playwright and Vite is ready.

Thank you

cc @sodatea

GreatAuk, segevfiner, sand4rt, azzamsa, and dafavio reacted with thumbs up emoji azzamsa reacted with heart emoji
Copy link
Member

@haoqunjiang haoqunjiang left a comment

Choose a reason for hiding this comment

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

The CI is failing. I think we need to also skip playwright tests in scripts/test.mjs since it requires different environment.

@mxschmitt mxschmitt force-pushed the feature-add-playwright branch 2 times, most recently from 2b8a7a1 to 935b7b4 Compare March 11, 2022 13:47
Copy link
Contributor Author

Applied the review feedback, sorry for the late response! Not sure if we should skip it inside the integration tests or run them - with my new implementation we run them.

(Would also be awesome if you could "lax" the GitHub Action permissions a big, so you don't have to approve all the time here)

Copy link
Member

@haoqunjiang haoqunjiang left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.

I've tried the PR locally and it's quite a joy to test it.
Just a few nitpicks that needs to be improved.

@@ -1,6 +1,6 @@
{
"scripts": {
"test:unit": "vitest --environment jsdom"
"test:unit": "vitest --environment jsdom --root src/"
Copy link
Member

@haoqunjiang haoqunjiang Apr 18, 2022

Choose a reason for hiding this comment

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

Why changing this line?

Copy link
Contributor Author

@mxschmitt mxschmitt Apr 19, 2022

Choose a reason for hiding this comment

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

Otherwise vitest tries to process the playwright test files as well, this won't work and lead to errors. Maybe there is a better way of handling that?

Copy link

Choose a reason for hiding this comment

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

Hello! It seems that this addition introduces a problem. When running Vitest it tries to create a results.json inside its cache directory. This directory is defined as resolve(root, slash(dir || 'node_modules/.vitest')). This means that with --root src/ added, Vitest creates the results.json inside the <project_root>/src/node_modules/.vitest instead of the actual node_modules directory (<project_root>/node_modules/.vitest).

@mxschmitt mxschmitt force-pushed the feature-add-playwright branch 2 times, most recently from 72b2491 to b2a32e5 Compare April 19, 2022 10:07
Copy link
Contributor Author

Sorry for the late review.

I've tried the PR locally and it's quite a joy to test it. Just a few nitpicks that needs to be improved.

No worries, was out of office myself so I did not notice at all. Applied the suggestions. Lets see what results the CI will give us.

Copy link

Out of curiosity, what's the current status on this? Playwright definitely is an interesting alternative to Cypress, so having it in there as an easy-to-access option seems lovely.

GreatAuk reacted with thumbs up emoji

Copy link

sand4rt commented Sep 4, 2022

@mxschmitt I wonder why this PR is closed?😕

@mxschmitt mxschmitt force-pushed the feature-add-playwright branch 3 times, most recently from f116e99 to d0a19ee Compare September 4, 2022 19:17
Copy link
Contributor Author

mxschmitt commented Sep 4, 2022
edited
Loading

@sodatea sorry for the huge delay, I was partially ooo back then and then I forgot about it.

Would be nice if you could enable GitHub Actions for contributors, this makes me easier to ensure its all green instead of having to wait for a maintainer to click approve.

I hope I addressed the comments, if not, please let me know!

The updates Playground PR are here: vuejs/create-vue-templates#1


@sand4rt I rebased it!

azzamsa reacted with thumbs up emoji sand4rt, stefnotch, and azzamsa reacted with heart emoji azzamsa reacted with eyes emoji

Copy link
Member

@haoqunjiang haoqunjiang left a comment

Choose a reason for hiding this comment

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

It looks good to me now.
Thank you for putting so much effort into this PR!

I'm merging the PR today.

But FYI, before cutting the next release, I'd like to make some adjustments to the workflow of this repo, the tsconfigs, and the CLI prompts, which would take some time.
So I expect this feature to be included in the October release.

sand4rt, mxschmitt, and stefnotch reacted with hooray emoji
@haoqunjiang haoqunjiang merged commit fcc6447 into vuejs:main Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haoqunjiang haoqunjiang haoqunjiang approved these changes

+1 more reviewer

@ptrgast ptrgast ptrgast left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /