-
Notifications
You must be signed in to change notification settings - Fork 5.4k
feat: improve yarn prepare scripts #6609
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #6609 +/- ##
==========================================
- Coverage 60.14% 60.09% -0.06%
==========================================
Files 719 719
Lines 21155 21143 -12
Branches 6980 6974 -6
==========================================
- Hits 12723 12705 -18
- Misses 8356 8360 +4
- Partials 76 78 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2 flaky tests on run #11209 ↗︎Details:
|
|||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Permit2 > swaps after handling user rejection of approvals and signatures |
Output
Screenshots
Video
|
|
swap/swap.test.ts • 1 flaky test • e2e
| Test | Artifacts | |
|---|---|---|
| Swap > Swap on main page > swaps ETH for USDC |
Output
Screenshots
Video
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
scripts/prepare.js
Outdated
| @@ -0,0 +1,36 @@ | |||
| const { spawn } = require('child_process') | |||
|
|
|||
| function runCommand(command, args) { | |||
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.
These will always be yarn scripts, so you can simplify this by omitting yarn:
| function runCommand(command, args) { | |
| function runYarnScript(command) { |
scripts/prepare.js
Outdated
| if (code !== 0) { | ||
| return reject(new Error(`${command} ${args} exited with code ${code}`)) | ||
| } | ||
| console.log(`${command} ${args} completed successfully`) |
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 think yarn will itself log when a command is completed. How much of the yarn command log is duplicated here? Do you even need to log when a command is initiated?
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 think the init log is useful to remind you what is executing under the hood.
Yarn doesn't log on its own when these child processes complete individually.
scripts/prepare.js
Outdated
|
|
||
| // Log errors | ||
| childProcess.stderr.on('data', (data) => { | ||
| console.error(`Error during execution of [${command}]:\n${data}`) |
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 still needs to be split and rejoined into lines like stdout.
This makes independent prepare scripts run in parallel which speeds up the default
yarncommand--it also gives labeled logs which should make issues easier to debug.