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

[BUG] Docs give misleading information about CircleCI that breaks components tests #19838

Closed
oriSomething opened this issue Jan 3, 2023 · 7 comments

Comments

@oriSomething
Copy link
Contributor

Context:

  • Playwright Version: next (both in latest)
  • Operating System: Circle CI - mcr.microsoft.com/playwright:v1.29.0-focal
  • Node.js version: What's in mcr.microsoft.com/playwright:v1.29.0-focal
  • Browser: All
  • Extra: CI

Code Snippet

This line in the docs that suggest the NODE_ENV=development is needed in CI is misleading:

  1. It works without it
  2. It's actually fails react component build with vite 4

Describe the bug

NODE_ENV=development causes tests with @playwright/experimental-ct-react to timeout due to wrong complication. Because docs suggest to do that in CI, it causes error that hard to link to wrong docs config

@mxschmitt
Copy link
Member

I never have used CircleCI myself. What happens if you don't specify it? Does it only install prod dependencies? I tried to find out why we have it there in the first place, but looks like we have it there since 2+ years.

@oriSomething
Copy link
Contributor Author

The project contains only devDependencies. It works fine even with NODE_ENV=production which is what we are using. But I must say we use yarn 3.

NODE_ENV:

  • works: nothing, production
  • doesn't work: development

@mxschmitt
Copy link
Member

Looks then like something specific to Yarn 3?

@oriSomething
Copy link
Contributor Author

If you want I'll try to make a repo to check npm. But I doubt it. You don't need NODE_ENV=development to install devDependencies or run bin of devDependencies

@mxschmitt
Copy link
Member

mxschmitt commented Jan 5, 2023

I think we are fine with removing the two lines. From my understanding it makes no difference to npm ci/install.

I tried to trace it back where it comes from (git blame) and its very old: #605.

Feel free to raise a PR with removing the two lines. (I can also change it if you're not comfortable with creating a PR.)

Its definitely wrong to recommend setting this env var for the whole pipeline.

oriSomething added a commit to oriSomething/playwright that referenced this issue Jan 5, 2023
Issue microsoft#19838
NODE_ENV=development causes experimental React component tests to fail with timeout.
But doesn't necessary to set to begin with
@oriSomething
Copy link
Contributor Author

I've made a PR #19887
I know you have very little time to deal with every issue 🙏

@mxschmitt
Copy link
Member

Thanks!

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

No branches or pull requests

2 participants