-
Notifications
You must be signed in to change notification settings - Fork 431
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
fix(ci): align pnpm install across all workflows #5920
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
No changes to documentation |
e2ee755
to
0688101
Compare
Component Testing Report Updated Mar 8, 2024 4:30 PM (UTC)
|
8396231
to
f0a3227
Compare
b012966
to
cd193b8
Compare
- name: Install project dependencies | ||
run: pnpm install | ||
|
||
- name: Build CLI |
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 was probably just leftover cruft, and there was no point in running build during the install step, so moved building to right before running the tests below
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.
actually I misunderstood this, so the way this is working is it's running the build step in install job, so it builds for node18
and node20
then it caches the build and passes it to the test job which runs for a mix of node + shard matrix
if the build
step is still needed we should keep the cache so it doesn't need to build on each matrix which would take quite long for 4 shards if cache is not available.
I thought the build step isn't required at all but maybe that will not work. Happy to chat more on this
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 thought the build step isn't required at all
You're right! Jest is configured to use the dev aliases, so tests can run from source. Although discovered that the validation tests, requires these aliases to be registered inside the worker thread that it runs.
Opened a separate PR to address this here: #5927
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.
Modified this PR to instead remove build entirely from the test workflow, so it should run a lot faster now.
cd193b8
to
5d3fc77
Compare
Unit tests doesn't require packages to be built since jest is configured to use the devAliases, routing imports from `<package>` to `<package>/src`.
5d3fc77
to
b0f8418
Compare
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.
very nice thank you for doing it.
Small note the install job and the cleanup jobs can be removed for the test action since they are not doing anything
c310516
to
fe46f4c
Compare
* fix(ci): align pnpm install across all workflows * fix(ci): skip running build before unit tests Unit tests doesn't require packages to be built since jest is configured to use the devAliases, routing imports from `<package>` to `<package>/src`. * fixup! fix(ci): skip running build before unit tests
Description
This aligns how we install dependencies across all our workflow definitions. It's using the same approach as @binoy14 previously added for the e2e test workflow.
What to review
--config.ignore-scripts=true
when running pnpm install? (side note: why theconfig.
-prefix and not just--ignore-scripts
?)Testing
Notes for release
n/a internal