fix(reporter): honor NO_COLOR in list/line/dot reporters#40903
Open
williamdes wants to merge 1 commit into
Open
fix(reporter): honor NO_COLOR in list/line/dot reporters#40903williamdes wants to merge 1 commit into
williamdes wants to merge 1 commit into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
dgozman
reviewed
May 19, 2026
Collaborator
dgozman
left a comment
dgozman
left a comment
There was a problem hiding this comment.
Could you please file a new issue for this? All contributions to Playwright require an issue, and this PR agrees that the original issue was already fixed anyway.
Collaborator
|
Let me close this until we figure out the exact issue this is fixing, and decide on the course of action there. |
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
|
@williamdes Reopened. Could you please limit this PR to supporting |
williamdes
force-pushed
the
fix/reporter-force-color-implies-non-tty
branch
from
bb4ab57 to
23bcfdc
Compare
williamdes
changed the title
williamdes
commented
May 20, 2026
Author
williamdes
left a comment
williamdes
left a comment
There was a problem hiding this comment.
I did a final review of the work and it looks okay by my review standards
Let me know
dgozman
reviewed
May 20, 2026
Per https://no-color.org/, any non-empty NO_COLOR should disable ANSI color output. The bundled chalk already honors it internally, but code in base.ts that reads `useColors` won't — so e.g. piping coloured test stdout through StripAnsiStream only kicks in when FORCE_COLOR=0 is also set. Add NO_COLOR to the existing useColors disabling branch so NO_COLOR=1 alone disables colors, matching chalk's behavior and the no-color.org convention. isTTY is left unchanged — no-color.org makes no claims about terminal shape, only about colors. Test in reporter-list.spec.ts asserts NO_COLOR=1 strips ANSI from test stdout, mirroring the existing FORCE_COLOR=0 test. Docs in test-reporters-js.md gain a NO_COLOR row in all three reporter tables (list / line / dot). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
williamdes
force-pushed
the
fix/reporter-force-color-implies-non-tty
branch
from
23bcfdc to
2f9ed6e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Per the NO_COLOR convention, any non-empty
NO_COLORvalue should disable ANSI color output. Playwright's bundled chalk honors that internally, but code inpackages/playwright/src/reporters/base.tsthat consultsuseColorsdoesn't — so e.g. theStripAnsiStreamwrapper introduced in #40688 only kicks in whenFORCE_COLOR=0is also set.What
NO_COLORadded to the existinguseColorsdisabling branch interminalScreen. Any non-emptyNO_COLORnow disables colors, matching chalk's behavior and the no-color.org spec.isTTYis left unchanged — no-color.org makes no claims about terminal shape, only about colors. (An earlier revision of this PR also gatedisTTYonNO_COLOR; reverted per review feedback.)Tests
One case added in
reporter-list.spec.tsmirroring the existingFORCE_COLOR=0test: assertsNO_COLOR=1strips ANSI from test stdout / stderr.Docs
docs/src/test-reporters-js.md— list/line/dot reporter tables now include a dedicatedNO_COLORrow pointing at no-color.org.Refs
Fixes #40904