fix: set chalk.level manually to prevent FORCE_COLOR leaks
#14396
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
The changes are breaking as the checks on e299a06 clearly demonstrate: tests are broken when testing Jest itself and expecting colored output. Update: Now, the color support is deduced from the environment where Jest is running (unless |
package.json
Outdated
| @@ -93,7 +93,7 @@ | |||
| "clean-e2e": "node ./scripts/cleanE2e.mjs", | |||
| "crowdin:upload": "echo 'Uploading sources to Crowdin' && crowdin upload sources --config ./crowdin.yaml", | |||
| "crowdin:download": "echo 'Downloading translations from Crowdin' && crowdin download --config ./crowdin.yaml", | |||
| "jest": "node ./packages/jest-cli/bin/jest.js", | |||
| "jest": "FORCE_COLOR=1 node ./packages/jest-cli/bin/jest.js", | |||
There was a problem hiding this comment.
On CI the test-ci-partial:parallel script is used:
jest/.github/workflows/test.yml
Line 37 in 0fd5b1c
It includes the --color flag, so all should work without FORCE_COLOR=1 adding:
Line 107 in 0fd5b1c
There was a problem hiding this comment.
@mrazauskas This is what you said in #14391 (comment):
The trick is that
--colorsis not passed anywhere and does not exist in Jest’s logic at all.chalksimply checks if--colors(or any numerous variation of it) is present inprocess.argv.
I actually think it should stay this way. In my opinion, having --color and the like have the same effect as setting FORCE_COLOR, or introducing any other means to set the color level for both the framework and the tests at the same time would be a bad decision because the uses of colors in the two have completely different meanings.
One might want to test colored output of a program that can produce such output in an environment that does not support colored output at all, and that would be perfectly valid. One might also want to ignore output coloring in tests and only focus on the contents of the output in an environment that supports colored output, but then using --no-color for this would be strange because that would also disable colors in the framework output, which we don't want.
Actually, my original solution with the FORCE_COLOR=1 prefix is also bad with respect to what I have just explained since it leaves no option to disable colors in the framework output.
Another problem I noticed is that if tests were to be run in an environment with no color support, then (almost?) all snapshot tests would fail because snapshots for the repo are apparently produced in an environment where there is such support, and the current behavior is essentially to simply let tests deduce the color level from the environment.
I solved both problems by adding a setup file to jest.config.mjs in which FORCE_COLOR is set to '1' for all tests, see b3558b7. (The configuration is only for internal Jest testing, so #14391 would still be fixed by merging this PR.)
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
The original issue is still not resolved. I might come back to this PR later to add tests and have a look at what else I can improve. |
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
1 similar comment
|
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #14391.