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: make tests OS agnostic #616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lifeiscontent
Copy link
Contributor

@lifeiscontent lifeiscontent commented Apr 29, 2024

What I did

  1. Update Playwright to run tests across MacOS and Linux
  2. Update Playwright to run across 4 different shards to speed up test runtime
  3. Update Playwright to use ./tests/integration as when using VSCode is provides a nice icon to the integration folder if using an icon pack
  4. Fix Playwright report merging
  5. Update tests to handle keyboard interactions across various OS types
  6. Use corepack in CI to install pnpm
  7. Use process.env.NODE_ENV === 'production' to determine if React is running in Strict mode rather than process.env.CI
  8. add favicon to playground to stop showing 404 errors in playwright
  9. fix typo of isStrictMode

closes #379

@lifeiscontent lifeiscontent force-pushed the chore/make-tests-os-agnostic branch 5 times, most recently from 5187716 to 5dac34b Compare April 29, 2024 16:06
Copy link
Owner

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Should we run the same test suites on window as well? Also, let's not change any package code in this PR.

tests/integrations/async-validation.spec.ts Outdated Show resolved Hide resolved
@lifeiscontent lifeiscontent force-pushed the chore/make-tests-os-agnostic branch 10 times, most recently from 0329e2f to d15640b Compare May 8, 2024 08:43
@lifeiscontent
Copy link
Contributor Author

@edmundhung I'm not sure if its worth it, the E2E tests with MacOS + Windows increase by more than double in terms of execution time, if you want to proceed with it anyway, I'll update to run MacOS again I tried to get Windows to run, but it looks like the playground Remix app doesn't run on windows currently 😅

@lifeiscontent lifeiscontent force-pushed the chore/make-tests-os-agnostic branch 2 times, most recently from 639cd33 to b106f55 Compare May 8, 2024 08:56
Copy link
Owner

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

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

Just one minor issue with the folder structure changes. Otherwise looks good to me. Thank you!

playwright.config.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
@edmundhung
Copy link
Owner

@edmundhung I'm not sure if its worth it, the E2E tests with MacOS + Windows increase by more than double in terms of execution time, if you want to proceed with it anyway, I'll update to run MacOS again I tried to get Windows to run, but it looks like the playground Remix app doesn't run on windows currently 😅

We can run them in parallel with GitHub action, right? I am fine to merge it as is but it would definitely be a big plus to be able to test on all 3 platforms.

@lifeiscontent lifeiscontent force-pushed the chore/make-tests-os-agnostic branch 2 times, most recently from 74d7fcd to ec2e1fa Compare May 9, 2024 11:14
@lifeiscontent lifeiscontent force-pushed the chore/make-tests-os-agnostic branch 3 times, most recently from 0018033 to afe7117 Compare May 16, 2024 12:39
@lifeiscontent
Copy link
Contributor Author

@edmundhung this is ready for review now, just need to update the required status checks in GitHub's Settings to the new names

@lifeiscontent
Copy link
Contributor Author

lifeiscontent commented May 16, 2024

@edmundhung using sharding in Playwright the total test time has slightly increased by a few seconds, so you won't see too much of a longer wait 🎉

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.

None yet

2 participants