chore: split output clients by capabilities and base dir #34135
Conversation
pavelfeldman
force-pushed
the
terminal_screen
branch
from
341e60d to
fe12c7e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| githubLogger = new GitHubLogger(); | ||
|
|
||
| constructor(options: { omitFailures?: boolean } = {}) { | ||
| super(options); | ||
| this.screen.colors = noColors; |
There was a problem hiding this comment.
To avoid changing the global terminalScreen.colors and messing up other reporters:
| this.screen.colors = noColors; | |
| this.screen = { ...this.screen, colors: noColors }; |
| @@ -222,7 +222,7 @@ class JSONReporter implements ReporterV2 { | |||
| } | |||
|
|
|||
| private _serializeError(error: TestError): JSONReportError { | |||
| return formatError(error, true); | |||
| return formatError(nonTerminalScreen, error); | |||
There was a problem hiding this comment.
This is a behavior change. Previously, we were always coloring the code snippet, and now we do it conditionally based on isTTY/PLAYWRIGHT_FORCE_TTY/FORCE_COLOR/DEBUG_COLORS.
Noticing here to figure out whether this is intentional.
There was a problem hiding this comment.
I don't think this changed - previously formatError was using colors (not realColors), so it was respecting the env variables.
|
|
||
| type MarkdownReporterOptions = { | ||
| configDir: string, | ||
| outputFile?: string; | ||
| }; | ||
|
|
||
|
|
||
| class MarkdownReporter extends BaseReporter { | ||
| class MarkdownReporter extends TerminalReporter { |
There was a problem hiding this comment.
This reporter seems to be a non-terminal one. I think it is more like the json/junit.
There was a problem hiding this comment.
That's true, but it inherited BaseReporter, which was a Terminal reporter. I can change it in a follow-up.
| @@ -160,6 +160,6 @@ class ListModeReporter implements ReporterV2 { | |||
|
|
|||
| onError(error: TestError) { | |||
| // eslint-disable-next-line no-console | |||
| console.error('\n' + formatError(error, false).message); | |||
| console.error('\n' + formatError(terminalScreen, error).message); | |||
There was a problem hiding this comment.
It looks like we did not want colors here at all, which seems consistent with plain console.log() above instead of using formatTitle(). Should we have a separate noColorsScreen?
There was a problem hiding this comment.
I think ListModeReporter is a kind of a terminal reporter, so why not keep it colorful?
| @@ -95,7 +95,7 @@ export function createErrorCollectingReporter(writeToConsole?: boolean): ErrorCo | |||
| onError(error: TestError) { | |||
| errors.push(error); | |||
| if (writeToConsole) | |||
| process.stdout.write(formatError(error, colors.enabled).message + '\n'); | |||
| process.stdout.write(formatError(terminalScreen, error).message + '\n'); | |||
There was a problem hiding this comment.
Should this be nonTerminalScreen instead?
There was a problem hiding this comment.
writeToConsole is true only for run-dev-server and clear-cache, which both are terminal. But I'm happy to make it accept the screen since it depends on the call context.
pavelfeldman
force-pushed
the
terminal_screen
branch
from
fe12c7e to
2e31b70
Compare
Test results for "tests 1"3 failed 7 flaky37512 passed, 654 skipped Merge workflow run. |
Fixes #34125