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

feat(playwright): reuse existing server #881

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

Conversation

Barbapapazes
Copy link

@Barbapapazes Barbapapazes commented Jul 2, 2024

related to #803

Hello 👋,

I was trying this module to write tests cause I really think tests are important.

But, in development, this module, e2e with Playwright, is slow. So slow that it creates too much friction to write tests and I think a lot of people will give up on writing tests because of that.

In the meantime, I use Playwright at work for e2e testing on an Angular app and it's fast. It's faster than unit tests with Jest and it's possible make test-first development with it.

After deep diving into the module, I found that for each test and each run a new app is built and started. This is the main reason for the slowness. While I understand this behavior on a CI pipeline, I don't think it's necessary on development. This is too much overhead for a little (or no) gain.

So I added a new option (similar to the existing one in Playwright, https://playwright.dev/docs/test-webserver#configuring-a-web-server) to reuse a dev server for all tests. This speeds up the tests a lot and makes it possible to use test-first development with Playwright.

While writing this, I realized that it could be possible to reuse the existing options instead of add a new one in the use.nuxt object. I'll try to do that in this PR. => this does not seems possible microsoft/playwright#7267

Maybe we could just disable the Nuxt integration to be able to reuse existing options

src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/setup/index.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
src/core/server.ts Outdated Show resolved Hide resolved
@Barbapapazes

This comment was marked as outdated.

@Barbapapazes Barbapapazes marked this pull request as ready for review July 11, 2024 16:56
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 11, 2024
@dosubot dosubot bot added the enhancement New feature or request label Jul 11, 2024
@Barbapapazes
Copy link
Author

Hey 👋, @danielroe, I think this PR is ready for review. I put 2 todos inside the code that are more questions than todos. I'd like to hear what you think.

@anPalmer
Copy link

@danielroe Is there any particular reason this one can’t be merged? Is it just the failure on Windows that requires some investigation? It’d be such an improvement for our project with lots of playwright tests. I’d be willing to take a crack at figuring it out. Thank you! 🙏

@danielroe
Copy link
Member

I didn't merge it because I thought that #803 would cover the situation.

The issue that needs to be handled here would be - only reusing a server that matches the configuration of the setup() call. So we might need to have (and persist) several servers. Maybe using a hash to identify which one is which, and then gracefully shutting them down when tests are done.

Also we'd need to handle potential mismatches between build/dev server (or gracefully rebuild a server when we detect changes within the source of that server - using a builder:watch hook - but not rebuilding when test files change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants