Skip to content

test: isolate color environment in configureOutput tests - #2608

Closed
nrps9909 wants to merge 2 commits into
tj:developfrom
nrps9909:codex/test-color-env-isolation
Closed

nrps9909 wants to merge 2 commits into
tj:developfrom
nrps9909:codex/test-color-env-isolation

Conversation

@nrps9909

@nrps9909 nrps9909 commented Sep 5, 2026 •

Copy link
Copy Markdown

Running the tests from a shell with NO_COLOR=1 makes the two configureOutput force-color cases fail, even though Commander correctly gives NO_COLOR precedence. With FORCE_COLOR=0, the CLICOLOR_FORCE case also inherits a conflicting setting. The tests currently save and replace only the variable they are testing.

Clear all three color variables before each subtest and restore their original values once in after when the group finishes, so inherited settings cannot change the assertions and a failed assertion still restores the environment. This changes only test setup and cleanup.

Validation on develop 97411c6:

  • Reproduced two failures with NO_COLOR=1 node --test tests/command.configureOutput.test.js before the change.
  • The focused file passes with no color variables, each of NO_COLOR=1, FORCE_COLOR=0, FORCE_COLOR=1, CLICOLOR_FORCE=1, and all three conflicting settings together.
  • npm test: 1,420 passing with the original NO_COLOR=1 environment.
  • npm run check: types, lint and formatting pass.
  • The focused file also passes on the minimum supported Node 22.12.0.

Suggested changelog summary: isolate configureOutput tests from inherited color environment variables.

Implemented and validated with Codex.

@SuperMo0

SuperMo0 commented Sep 5, 2026

Copy link
Copy Markdown

Why use afterEach instead of after? The beforeEach is going to delete the keys anyway, so there is no need to reset them to the original value after every iteration.

@nrps9909

nrps9909 commented Sep 5, 2026

Copy link
Copy Markdown
Author

afterEach was more cleanup than this serial group needs. I've changed it to t.after() in f57c87b: beforeEach still clears all three controls before each case, and the original environment is restored once when the group finishes, including if a subtest fails. This also matches the cleanup scope used in useColor.test.js.

Rechecked all six environment configurations, the full 1,420-test suite, types/lint/format, and the 24-test focused file on Node 22.12.0. All pass.

@shadowspawn

Copy link
Copy Markdown
Collaborator

Thank-you for the AI disclosure in the Pull Request.

I do not want to make the change at this time, and instead I am treating it as a support example for #2549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants