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

test(environment): add environment playground #16299

Merged
merged 18 commits into from
Mar 29, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Mar 29, 2024

Description

I added a minimal ssr environment example including my normal e2e testing setup.
Probably it's not ideal to have react specific playground (even though it doesn't use react plugin) and also playwright based testing, but I thought it might help to have this while iterating on environment PR.

This also includes a build fix from PR #16298.

Copy link

stackblitz bot commented Mar 29, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

},
},

// [feedback] should preview automatically pick up environments.client.build.outDir?
Copy link
Member

Choose a reason for hiding this comment

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

We didn't work yet on the preview side, but I was thinking we should have a PreviewEnvironment and make preview work for the whole app in the same way that we are now letting vite build build the whole app. This is something that was requested by framework authors before and we have been slowly making Preview work closer to the dev server. I think vite should work at the app level in all commands.

@patak-dev
Copy link
Member

patak-dev commented Mar 29, 2024

This is great! I think we should merge it as it will be really helpful. But first, I think it is worth refactoring the test to use our regular e2e test infra.
And we should merge #16301 with the fix only, removing the playground.

@hi-ogawa
Copy link
Collaborator Author

Thanks for the review! Yeah, I'll replace playwright with vitest e2e.

For the other PR #16301, I just removed the playground part.

@hi-ogawa
Copy link
Collaborator Author

I rewrote it in vitest e2e, but maybe this might be a little unusual setup. A few ideas I have is:

  • use expect assertions from @playwright/test
  • custom serve.ts with execa to directly invoke pnpm dev/build/preview

To me, this approach feels right as e2e, but probably this is a bit opinionated. Please let me know if it should be further aligned with existing e2e tests.


Also it looks like typecheck is failing on CI, but not sure what's wrong. When I run pnpm run typecheck locally, it's working:

$ pnpm run typecheck

> @vitejs/vite-monorepo@ typecheck /home/hiroshi/code/others/vite
> tsc -p scripts --noEmit && pnpm -r --parallel run typecheck

Scope: 212 of 213 workspace projects
packages/create-vite typecheck$ tsc --noEmit
playground typecheck$ tsc --noEmit
packages/vite typecheck$ tsc --noEmit
packages/create-vite typecheck: Done
playground typecheck: Done
packages/vite typecheck: Done

@hi-ogawa hi-ogawa marked this pull request as ready for review March 29, 2024 06:59
@patak-dev
Copy link
Member

The setup looks good to me, but I think we should align with the rest. And we can discuss later about why you think this is better and maybe move all the playgrounds to implement some of these ideas. Except if there is a very good reason that prevents you to aligning.

@hi-ogawa
Copy link
Collaborator Author

Except if there is a very good reason that prevents you to aligning.

Actually test-serve just worked as is, but for test-build to work, it needs to change vitestSetup.ts since it uses programmatic way of running vite build and vite preview.

The reason why I like this setup is it tests the same layer as what users would use from cli. But, non-programmatic usage cannot directly assert things only accessible programmatically like viteServer etc... exported fom vitestSetup.ts, so it has less flexibility.

Should I try programmatic usage of vite builder?

@patak-dev
Copy link
Member

Yes, I think programatic usage of vite builder would be good here. That is a good test too.
We have a CLI playground where we could test later the CLI side of things, but it is a very thin layer, we don't miss that much coverage by using ViteBuilder directly.

@hi-ogawa
Copy link
Collaborator Author

OK, actually it was simpler than how I struggled with custom serve.ts. I exported createViteBuilder and just used it like build --all CLI.

package.json Outdated
@@ -42,6 +42,7 @@
"devDependencies": {
"@babel/types": "^7.24.0",
"@eslint-types/typescript-eslint": "^7.2.0",
"@playwright/test": "^1.42.1",
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this one now, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be a separate discussion too, but I think we could consider using expect from @playwright/test where it's appropriate since it has some neat feature to write robust assertions.

For now, I'll revert this back too. Let me check.

@patak-dev
Copy link
Member

Awesome! Thanks for adapting the test 🙏🏼
The fails are the same as we have right now in feat/environment-api. @sheremet-va should we skip this test for now there?

@@ -1,7 +1,7 @@
import { resolve } from 'node:path'
import { defineConfig } from 'vitest/config'

const timeout = process.env.CI ? 50000 : 30000
const timeout = process.env.PWDEBUG ? Infinity : process.env.CI ? 50000 : 30000
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also added this to allow using playwright inspector without timeout.
For example, running this command will break at the first page.goto(..) and you can step debug and record with playwright browser window.

PWDEBUG=1 pnpm test-build playground/environment-react-ssr

@patak-dev
Copy link
Member

This is great!

@patak-dev patak-dev merged commit a5c7e4f into vitejs:feat/environment-api Mar 29, 2024
2 of 6 checks passed
@hi-ogawa hi-ogawa deleted the test-environment-ssr branch March 30, 2024 01:19
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

Successfully merging this pull request may close these issues.

2 participants