-
-
Notifications
You must be signed in to change notification settings - Fork 89
Avoid to repeat unnecessary build #543
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
Summary of ChangesHello @moreal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the project's build and test workflow by decoupling the build process from the test execution in individual packages. By leveraging Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request correctly separates the build and test steps to improve CI performance and prevent race conditions. By moving build commands to pretest scripts and disabling them during the recursive test run in mise.toml, the changes ensure that packages are built only once. The implementation is consistent across all packages and effectively solves the problem described. This is a great improvement for the project's development workflow.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
dahlia
left a comment
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.
Thanks!
Summary
This pull request is inspired from 4bca460 commit. 🙏🏻
While the
pnpm run -rcommand itself runs tasks sequentially according to dependencies, thepnpm buildit calls when running tests causes unnecessary and race-condition-inducing duplicate and parallel builds.So this pull request separates
pnpm buildfromtestscript aspretestscript. My intention is:pnpm buildwhen running for each test. For instance,cd packages/fedify && pnpm testshould runpnpm buildto build dependencies and itself.pnpm buildbymise task test, because thetesttask depends onpreparetask which builds packages by runningpnpm run -r build:self.Related issue
I couldn't find related issue when searching with:
Changes
List the specific modifications made in this PR.
Focus on what was changed without going into detail about impact.
pnpm buildscript in eachtestscript aspretestscript.mise task testignore pre/post script by--config.enable-pre-post-scripts=falseBenefits
While it's hard to calculate precisely, it will reduce not only the time it takes to run mise tests locally but also CI execution time.
In my local environment:
test:bunhas failed.In the GitHub Actions:
The above GitHub Actions example takes 9m 24s for running test:node
In this pull request, it takes 5m 33s.
Of course, this is not precise.
Additionally, it is expected to resolve the race conditions that occurred with the existing parallel builds.
Checklist
mise teston your machine?Additional notes
--config.option