feat(test runner): do not mock tty in the worker process (#30107)

This was historically done to make `console.log()` have colors. However, this makes any other code that checks `process.stdout.isTTY` incorrectly assume real TTY support. Node18 and Node20 now respect `FORCE_COLOR=1` in console, so our default behavior of forcing colors in the worker process just works out of the box. See nodejs/node#48034.
microsoft · Mar 25, 2024 · 911d8ef · 911d8ef
1 parent 4b3c596
commit 911d8ef
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 107 deletions.
diff --git a/packages/playwright/src/common/ipc.ts b/packages/playwright/src/common/ipc.ts
@@ -46,15 +46,7 @@ export type SerializedConfig = {
   compilationCache?: SerializedCompilationCache;
 };
 
-export type TtyParams = {
-  rows: number | undefined;
-  columns: number | undefined;
-  colorDepth: number;
-};
-
 export type ProcessInitParams = {
-  stdoutParams: TtyParams;
-  stderrParams: TtyParams;
   processName: string;
 };
 

diff --git a/packages/playwright/src/common/process.ts b/packages/playwright/src/common/process.ts
@@ -14,8 +14,7 @@
  * limitations under the License.
  */
 
-import type { WriteStream } from 'tty';
-import type { EnvProducedPayload, ProcessInitParams, TtyParams } from './ipc';
+import type { EnvProducedPayload, ProcessInitParams } from './ipc';
 import { startProfiling, stopProfiling } from 'playwright-core/lib/utils';
 import type { TestInfoError } from '../../types/test';
 import { serializeError } from '../util';
@@ -67,8 +66,6 @@ const startingEnv = { ...process.env };
 process.on('message', async (message: any) => {
   if (message.method === '__init__') {
     const { processParams, runnerParams, runnerScript } = message.params as { processParams: ProcessInitParams, runnerParams: any, runnerScript: string };
-    setTtyParams(process.stdout, processParams.stdoutParams);
-    setTtyParams(process.stderr, processParams.stderrParams);
     void startProfiling();
     const { create } = require(runnerScript);
     processRunner = create(runnerParams) as ProcessRunner;
@@ -117,40 +114,3 @@ function sendMessageToParent(message: { method: string, params?: any }) {
     // Can throw when closing.
   }
 }
-
-function setTtyParams(stream: WriteStream, params: TtyParams) {
-  stream.isTTY = true;
-  if (params.rows)
-    stream.rows = params.rows;
-  if (params.columns)
-    stream.columns = params.columns;
-  stream.getColorDepth = () => params.colorDepth;
-  stream.hasColors = ((count = 16) => {
-    // count is optional and the first argument may actually be env.
-    if (typeof count !== 'number')
-      count = 16;
-    return count <= 2 ** params.colorDepth;
-  })as any;
-
-  // Stubs for the rest of the methods to avoid exceptions in user code.
-  stream.clearLine = (dir: any, callback?: () => void) => {
-    callback?.();
-    return true;
-  };
-  stream.clearScreenDown = (callback?: () => void) => {
-    callback?.();
-    return true;
-  };
-  (stream as any).cursorTo = (x: number, y?: number | (() => void), callback?: () => void) => {
-    if (callback)
-      callback();
-    else if (y instanceof Function)
-      y();
-    return true;
-  };
-  stream.moveCursor = (dx: number, dy: number, callback?: () => void) => {
-    callback?.();
-    return true;
-  };
-  stream.getWindowSize = () => [stream.columns, stream.rows];
-}
diff --git a/packages/playwright/src/runner/processHost.ts b/packages/playwright/src/runner/processHost.ts
@@ -112,16 +112,6 @@ export class ProcessHost extends EventEmitter {
       return error;
 
     const processParams: ProcessInitParams = {
-      stdoutParams: {
-        rows: process.stdout.rows,
-        columns: process.stdout.columns,
-        colorDepth: process.stdout.getColorDepth?.() || 8
-      },
-      stderrParams: {
-        rows: process.stderr.rows,
-        columns: process.stderr.columns,
-        colorDepth: process.stderr.getColorDepth?.() || 8
-      },
       processName: this._processName
     };
 

diff --git a/tests/playwright-test/stdio.spec.ts b/tests/playwright-test/stdio.spec.ts
@@ -78,7 +78,14 @@ test('should ignore stdio when quiet', async ({ runInlineTest }) => {
   expect(result.output).not.toContain('%%');
 });
 
-test('should support console colors', async ({ runInlineTest }) => {
+test('should support console colors but not tty', {
+  annotation: [
+    { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/15366' },
+    { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/29839' },
+  ],
+}, async ({ runInlineTest, nodeVersion }) => {
+  test.skip(nodeVersion.major < 18, 'Node16 does not respect FORCE_COLOR in onsole');
+
   const result = await runInlineTest({
     'a.spec.js': `
       import { test, expect } from '@playwright/test';
@@ -90,31 +97,13 @@ test('should support console colors', async ({ runInlineTest }) => {
       });
     `
   });
-  expect(result.output).toContain(`process.stdout.isTTY = true`);
-  expect(result.output).toContain(`process.stderr.isTTY = true`);
+  expect(result.output).toContain(`process.stdout.isTTY = undefined`);
+  expect(result.output).toContain(`process.stderr.isTTY = undefined`);
   // The output should have colors.
   expect(result.rawOutput).toContain(`{ b: \x1b[33mtrue\x1b[39m, n: \x1b[33m123\x1b[39m, s: \x1b[32m'abc'\x1b[39m }`);
   expect(result.rawOutput).toContain(`{ b: \x1b[33mfalse\x1b[39m, n: \x1b[33m123\x1b[39m, s: \x1b[32m'abc'\x1b[39m }`);
 });
 
-test('should override hasColors and getColorDepth', async ({ runInlineTest }) => {
-  const result = await runInlineTest({
-    'a.spec.js': `
-      import { test, expect } from '@playwright/test';
-      test('console log', () => {
-        console.log('process.stdout.hasColors(1) = ' + process.stdout.hasColors(1));
-        console.log('process.stderr.hasColors(1) = ' + process.stderr.hasColors(1));
-        console.log('process.stdout.getColorDepth() > 0 = ' + (process.stdout.getColorDepth() > 0));
-        console.log('process.stderr.getColorDepth() > 0 = ' + (process.stderr.getColorDepth() > 0));
-      });
-    `
-  });
-  expect(result.output).toContain(`process.stdout.hasColors(1) = true`);
-  expect(result.output).toContain(`process.stderr.hasColors(1) = true`);
-  expect(result.output).toContain(`process.stdout.getColorDepth() > 0 = true`);
-  expect(result.output).toContain(`process.stderr.getColorDepth() > 0 = true`);
-});
-
 test('should not throw type error when using assert', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.spec.js': `
@@ -128,30 +117,3 @@ test('should not throw type error when using assert', async ({ runInlineTest })
   expect(result.output).not.toContain(`TypeError: process.stderr.hasColors is not a function`);
   expect(result.output).toContain(`AssertionError`);
 });
-
-test('should provide stubs for tty.WriteStream methods on process.stdout/stderr', async ({ runInlineTest }) => {
-  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/29839' });
-  const result = await runInlineTest({
-    'a.spec.ts': `
-      import { test, expect } from '@playwright/test';
-      import type * as tty from 'tty';
-      async function checkMethods(stream: tty.WriteStream) {
-        expect(stream.isTTY).toBe(true);
-        await new Promise<void>(r =>  stream.clearLine(-1, r));;
-        await new Promise<void>(r =>  stream.clearScreenDown(r));;
-        await new Promise<void>(r =>  stream.cursorTo(0, 0, r));;
-        await new Promise<void>(r =>  stream.cursorTo(0, r));;
-        await new Promise<void>(r =>  stream.moveCursor(1, 1, r));;
-        expect(stream.getWindowSize()).toEqual([stream.columns, stream.rows]);
-        // getColorDepth() and hasColors() are covered in other tests.
-      }
-      test('process.stdout implementd tty.WriteStream methods', () => {
-        checkMethods(process.stdout);
-      });
-      test('process.stderr implementd tty.WriteStream methods', () => {
-        checkMethods(process.stderr);
-      });
-    `
-  });
-  expect(result.exitCode).toBe(0);
-});