-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Deduplicate CI testing #4206
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
Deduplicate CI testing #4206
Conversation
|
|
Personally, I'd remove most of the ~25 different test scripts from |
This reverts commit 470b4fb.
|
...instead of like this: Open to a third way if it's possible to DRY that out without introducing a lot of indirection |
|
This should do! |
| trace: 'retain-on-failure' | ||
| } | ||
| }, | ||
| workers: process.env.CI ? 2 : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had removed this purposefully not long ago. a lot of the CIs have only 1 CPU and we were worried that having multiple workers was making them compete for CPU and causing timeouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test, but adding full parallelization to the tests (at the pnpm level) didn't cause contention to the point of increasing run time, in fact it reduced a few percent. This is also a "max workers" setting—shouldn't playwright still use 1 worker when there's only one cpu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think adding more workers would increase overall runtime, but it may make individual tests take longer to run
But anyway it looks like I may have been wrong that some environments only support a single CPU? I think I'm seeing now that most support 2, so this would be fine
Let's see if this helps. I'm not sure the port assignment will work.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0