-
Notifications
You must be signed in to change notification settings - Fork 0
fix: fix testStart #554
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: fix testStart #554
Conversation
Summary of ChangesHello @exKAZUu, 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 addresses an issue where the 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
このプルリクエストは、testStartコマンドにNODE_ENV=""を追加することで、テスト開始時の問題を修正するものです。コメントによると、これはBlitz.jsが本番モードで起動するのを防ぐためのようです。変更はBaseScriptsという基底クラスに行われており、Blitz.js以外のフレームワークにも影響を与える可能性があります。この点が意図したものでない場合、意図しない副作用を避けるために、変更をBlitz.js専用のスクリプトファイルに限定することを提案しました。
| // Use empty NODE_ENV to avoid "production" mode in Blitz.js. | ||
| return `NODE_ENV="" YARN concurrently --kill-others --raw --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; |
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.
この変更は基底クラスであるBaseScriptsに行われていますが、コメントではBlitz.jsに特化した理由が述べられています。もしこのNODE_ENVの設定がBlitz.jsでのみ必要な場合、他のフレームワーク(Next.js, Remixなど)への意図しない影響を避けるため、testStartメソッドをblitzScripts.ts内でオーバーライドする方が適切かもしれません。
もしこの変更がすべてのフレームワークにとって安全かつ有益である場合は、その旨をコメントに反映させることをご検討ください。例えば、「NODE_ENVが未設定の場合に一部のフレームワーク(Blitz.jsなど)が本番モードで起動するのを防ぐため」のように一般化するなどです。
提案 (もしBlitz.js専用の場合):
packages/wb/src/scripts/execution/blitzScripts.ts に以下のように追加します:
// (ファイルの先頭に以下を追加)
// import { checkAndKillPortProcess } from '../../utils/port.js';
// class BlitzScripts 内に以下を追加
override async testStart(project: Project, argv: ScriptArgv): Promise<string> {
await checkAndKillPortProcess(project.env.PORT, project);
// Use empty NODE_ENV to avoid "production" mode in Blitz.js.
return `NODE_ENV=\"\" YARN concurrently --kill-others --raw --success first \"${this.startDevProtected(project, argv)}\" \"${this.waitApp(project)}\"`;
}そして packages/wb/src/scripts/execution/baseScripts.ts の変更を元に戻します。
References
- レビューは日本語で行う必要があります。 (link)
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.
Pull request overview
This PR fixes the testStart method in the BaseScripts class by adding NODE_ENV="" to prevent Blitz.js from running in production mode during test execution. The change ensures that when tests are run, the development server starts in development mode rather than production mode, which is the intended behavior for testing.
Key Changes:
- Added
NODE_ENV=""environment variable prefix to thetestStartcommand - Added a comment explaining the purpose of the change
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use empty NODE_ENV to avoid "production" mode in Blitz.js. | ||
| return `NODE_ENV="" YARN concurrently --kill-others --raw --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; |
Copilot
AI
Dec 8, 2025
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.
[nitpick] Consider using NODE_ENV="development" instead of NODE_ENV="". An empty string might cause unexpected behavior in frameworks that explicitly check for values like "development", "production", or "test". Setting it to "development" would be more explicit and align better with the intent of running in development mode for testing purposes.
| // Use empty NODE_ENV to avoid "production" mode in Blitz.js. | |
| return `NODE_ENV="" YARN concurrently --kill-others --raw --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; | |
| // Use NODE_ENV="development" to avoid "production" mode in Blitz.js. | |
| return `NODE_ENV="development" YARN concurrently --kill-others --raw --success first "${this.startDevProtected(project, argv)}" "${this.waitApp(project)}"`; |
Close #
Self Check
All checks have passedon this page.WIP:.yarn test) passed.yarn lint) passed.