fix: reorganize the logic of color and remove the forced default values#946
fix: reorganize the logic of color and remove the forced default values#946
Conversation
fe06af7 to
ed55162
Compare
There was a problem hiding this comment.
Pull request overview
This PR simplifies color environment variable handling to align with vitest's behavior. Instead of always forcing FORCE_COLOR=1 for worker processes, rstest now only intervenes in agent environments (by disabling colors), and otherwise lets child processes decide based on their own environment detection.
Changes:
- Modified
getForceColorEnv()to only set color env vars in agent environments when user hasn't explicitly set them - Updated test to verify that
NO_COLORis passed through without addingFORCE_COLOR - Added new test to verify that rstest doesn't set color env vars by default (like vitest)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/utils/logger.ts | Simplified getForceColorEnv() logic to only intervene in agent environments when user hasn't set color env vars, removing the forced FORCE_COLOR=1 default |
| e2e/env/index.test.ts | Updated test description and added new test to verify default behavior matches vitest |
| e2e/env/fixtures/noColor.test.ts | Updated assertions to verify that FORCE_COLOR is not set when user provides NO_COLOR |
| e2e/env/fixtures/defaultColor.test.ts | New test fixture to verify that neither color env var is set by default |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should not set FORCE_COLOR when no color env is set by user', async ({ | ||
| onTestFinished, | ||
| }) => { | ||
| const { expectExecSuccess } = await runRstestCli({ | ||
| command: 'rstest', | ||
| args: ['run', 'defaultColor.test.ts'], | ||
| onTestFinished, | ||
| // Unset both color envs to test default behavior (like vitest) | ||
| unsetEnv: ['FORCE_COLOR', 'NO_COLOR'], | ||
| options: { | ||
| nodeOptions: { | ||
| cwd: join(__dirname, 'fixtures'), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await expectExecSuccess(); | ||
| }); |
There was a problem hiding this comment.
This test doesn't explicitly control agent detection (via RSTEST_NO_AGENT), which means it could behave differently if run in a CI environment that's detected as an agent. In agent environments, the new logic in getForceColorEnv() would set both NO_COLOR=1 and FORCE_COLOR=0, causing this test to fail. Consider adding 'RSTEST_NO_AGENT' to the unsetEnv array to ensure consistent behavior across different environments.
|
|
||
| await expectExecSuccess(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new getForceColorEnv() logic has a specific behavior for agent environments (setting NO_COLOR=1 and FORCE_COLOR=0), but there's no E2E test verifying this behavior. Consider adding a test that explicitly sets AI_AGENT environment variable and verifies that colors are disabled in worker processes unless the user explicitly sets color env vars. This would ensure the agent environment logic works as intended, as seen in e2e/reporter/md.test.ts where AI_AGENT is set to test agent behavior.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Simplify color environment variable handling to align with vitest's behavior.
New Color Logic
The new
getForceColorEnv()logic is straightforward:NO_COLOR=1andFORCE_COLOR=0(unless user explicitly set color env vars)This is consistent with how vitest handles color env vars - it simply passes through
process.envwithout forcing any color settings.Why prebundle Previously Passed (Accidentally)
In the original implementation (before the agent detection changes), rstest always set
FORCE_COLOR=1for worker processes:This caused prebundle's chalk color test to pass because rstest was forcing colors on, even in non-TTY CI environments. However, this was not intentional behavior - the test was relying on a side effect rather than explicitly requesting color output.
When running the same test with vitest, it fails for the same reason - vitest doesn't force
FORCE_COLOR=1either.Breaking Change for Downstream
Projects that relied on rstest automatically setting
FORCE_COLOR=1need to explicitly enable colors in their tests if needed:Related Links
Checklist