test: unflake fixtures test (#2313)

dgozman · web-flow · commit 48440f7ed76a · 2020-05-20T14:58:27.000-07:00
Drive-by: ensure we call onkill before manually exiting the process.
diff --git a/src/server/chromium.ts b/src/server/chromium.ts
@@ -142,6 +142,8 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
       pipe: true,
       tempDir: temporaryUserDataDir || undefined,
       attemptToGracefullyClose: async () => {
+        if ((options as any).__testHookGracefullyClose)
+          await (options as any).__testHookGracefullyClose();
         debugAssert(browserServer._isInitialized());
         // We try to gracefully close to prevent crash reporting and core dumps.
         // Note that it's fine to reuse the pipe transport, since
diff --git a/src/server/firefox.ts b/src/server/firefox.ts
@@ -137,6 +137,8 @@ export class Firefox extends AbstractBrowserType<FFBrowser> {
       pipe: false,
       tempDir: temporaryProfileDir || undefined,
       attemptToGracefullyClose: async () => {
+        if ((options as any).__testHookGracefullyClose)
+          await (options as any).__testHookGracefullyClose();
         debugAssert(browserServer._isInitialized());
         // We try to gracefully close to prevent crash reporting and core dumps.
         const transport = await WebSocketTransport.connect(browserWSEndpoint!, async transport => transport);
diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts
@@ -111,18 +111,21 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
   const downloadsPath = options.omitDownloads ? '' : await mkdtempAsync(DOWNLOADS_FOLDER);
 
   let processClosed = false;
-  const waitForProcessToClose = new Promise((fulfill, reject) => {
-    spawnedProcess.once('exit', (exitCode, signal) => {
-      logger._log(browserLog, `<process did exit ${exitCode}, ${signal}>`);
-      processClosed = true;
-      helper.removeEventListeners(listeners);
-      options.onkill(exitCode, signal);
-      // Cleanup as processes exit.
-      Promise.all([
-        options.omitDownloads ? Promise.resolve() : removeFolderAsync(downloadsPath),
-        options.tempDir ? removeFolderAsync(options.tempDir) : Promise.resolve()
-      ]).catch((err: Error) => console.error(err)).then(fulfill);
-    });
+  let fulfillClose = () => {};
+  const waitForClose = new Promise(f => fulfillClose = f);
+  let fulfillCleanup = () => {};
+  const waitForCleanup = new Promise(f => fulfillCleanup = f);
+  spawnedProcess.once('exit', (exitCode, signal) => {
+    logger._log(browserLog, `<process did exit ${exitCode}, ${signal}>`);
+    processClosed = true;
+    helper.removeEventListeners(listeners);
+    options.onkill(exitCode, signal);
+    fulfillClose();
+    // Cleanup as processes exit.
+    Promise.all([
+      options.omitDownloads ? Promise.resolve() : removeFolderAsync(downloadsPath),
+      options.tempDir ? removeFolderAsync(options.tempDir) : Promise.resolve()
+    ]).catch((err: Error) => console.error(err)).then(fulfillCleanup);
   });
 
   const listeners = [ helper.addEventListener(process, 'exit', killProcess) ];
@@ -145,12 +148,13 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     if (gracefullyClosing) {
       logger._log(browserLog, `<forecefully close>`);
       killProcess();
+      await waitForClose;  // Ensure the process is dead and we called options.onkill.
       return;
     }
     gracefullyClosing = true;
     logger._log(browserLog, `<gracefully close start>`);
     await options.attemptToGracefullyClose().catch(() => killProcess());
-    await waitForProcessToClose;
+    await waitForCleanup;  // Ensure the process is dead and we have cleaned up.
     logger._log(browserLog, `<gracefully close end>`);
   }
 
@@ -169,10 +173,12 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
         // the process might have already stopped
       }
     }
-    // Attempt to remove temporary profile directory to avoid littering.
+    // Attempt to remove temporary directories to avoid littering.
     try {
       if (options.tempDir)
         removeFolder.sync(options.tempDir);
+      if (!options.omitDownloads)
+        removeFolder.sync(downloadsPath);
     } catch (e) { }
   }
 
diff --git a/src/server/webkit.ts b/src/server/webkit.ts
@@ -125,6 +125,8 @@ export class WebKit extends AbstractBrowserType<WKBrowser> {
       pipe: true,
       tempDir: temporaryUserDataDir || undefined,
       attemptToGracefullyClose: async () => {
+        if ((options as any).__testHookGracefullyClose)
+          await (options as any).__testHookGracefullyClose();
         assert(transport);
         // We try to gracefully close to prevent crash reporting and core dumps.
         // Note that it's fine to reuse the pipe transport, since
diff --git a/test/fixtures.spec.js b/test/fixtures.spec.js
@@ -19,123 +19,161 @@ const path = require('path');
 const {spawn, execSync} = require('child_process');
 const {FFOX, CHROMIUM, WEBKIT, WIN, LINUX} = require('./utils').testOptions(browserType);
 
-async function testSignal(state, action, exitOnClose) {
-  const options = Object.assign({}, state.defaultBrowserOptions, {
-    handleSIGINT: true,
-    handleSIGTERM: true,
-    handleSIGHUP: true,
-    executablePath: state.browserType.executablePath(),
-    logger: undefined,
-  });
-  const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), path.join(state.playwrightPath, 'index-for-dev'), state.browserType.name(), JSON.stringify(options), exitOnClose ? 'true' : '']);
-  let wsEndPointCallback;
-  const wsEndPointPromise = new Promise(x => wsEndPointCallback = x);
-  let output = '';
-  let browserExitCode = 'none';
-  let browserSignal = 'none';
-  let browserPid;
-  res.stdout.on('data', data => {
-    output += data.toString();
-    // Uncomment to debug these tests.
-    // console.log(data.toString());
-    let match = output.match(/browserWS:(.+):browserWS/);
-    if (match)
-      wsEndPointCallback(match[1]);
-    match = output.match(/browserClose:([^:]+):([^:]+):browserClose/);
-    if (match) {
-      browserExitCode = match[1];
-      browserSignal = match[2];
-    }
-    match = output.match(/browserPid:([^:]+):browserPid/);
-    if (match)
-      browserPid = +match[1];
-  });
-  res.on('error', (...args) => console.log("ERROR", ...args));
-  const browser = await state.browserType.connect({ wsEndpoint: await wsEndPointPromise });
-  const promises = [
-    new Promise(resolve => browser.once('disconnected', resolve)),
-    new Promise(resolve => res.on('exit', resolve)),
-  ];
-  action(res, browserPid);
-  const [, exitCode] = await Promise.all(promises);
-  return { exitCode, browserSignal, browserExitCode, output };
+class Wrapper {
+  constructor(state, extraOptions) {
+    this._output = new Map();
+    this._outputCallback = new Map();
+
+    this._browserType = state.browserType;
+    const launchOptions = {...state.defaultBrowserOptions,
+      handleSIGINT: true,
+      handleSIGTERM: true,
+      handleSIGHUP: true,
+      executablePath: state.browserType.executablePath(),
+      logger: undefined,
+    };
+    const options = {
+      playwrightFile: path.join(state.playwrightPath, 'index-for-dev'),
+      browserTypeName: state.browserType.name(),
+      launchOptions,
+      ...extraOptions,
+    };
+    this._child = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), JSON.stringify(options)]);
+    this._child.on('error', (...args) => console.log("ERROR", ...args));
+    this._exitPromise = new Promise(resolve => this._child.on('exit', resolve));
+
+    let outputString = '';
+    this._child.stdout.on('data', data => {
+      outputString += data.toString();
+      // Uncomment to debug.
+      // console.log(data.toString());
+      let match;
+      while (match = outputString.match(/\(([^()]+)=>([^()]+)\)/)) {
+        const key = match[1];
+        const value = match[2];
+        this._addOutput(key, value);
+        outputString = outputString.substring(match.index + match[0].length);
+      }
+    });
+  }
+
+  _addOutput(key, value) {
+    this._output.set(key, value);
+    const cb = this._outputCallback.get(key);
+    this._outputCallback.delete(key);
+    if (cb)
+      cb();
+  }
+
+  async out(key) {
+    if (!this._output.has(key))
+      await new Promise(f => this._outputCallback.set(key, f));
+    return this._output.get(key);
+  }
+
+  async connect() {
+    const wsEndpoint = await this.out('wsEndpoint');
+    const browser = await this._browserType.connect({ wsEndpoint });
+    this._exitAndDisconnectPromise = Promise.all([
+      this._exitPromise,
+      new Promise(resolve => browser.once('disconnected', resolve)),
+    ]).then(([exitCode]) => exitCode);
+  }
+
+  child() {
+    return this._child;
+  }
+
+  async childExitCode() {
+    return await this._exitAndDisconnectPromise;
+  }
+}
+
+async function setup(state, options = {}) {
+  const wrapper = new Wrapper(state, options);
+  await wrapper.connect();
+  return wrapper;
 }
 
 describe('Fixtures', function() {
   it.slow()('should close the browser when the node process closes', async state => {
-    const result = await testSignal(state, child => {
-      if (WIN)
-        execSync(`taskkill /pid ${child.pid} /T /F`);
-      else
-        process.kill(child.pid);
-    });
-    expect(result.exitCode).toBe(WIN ? 1 : 0);
+    const wrapper = await setup(state);
+    if (WIN)
+      execSync(`taskkill /pid ${wrapper.child().pid} /T /F`);
+    else
+      process.kill(wrapper.child().pid);
+    expect(await wrapper.childExitCode()).toBe(WIN ? 1 : 0);
     // We might not get browser exitCode in time when killing the parent node process,
     // so we don't check it here.
   });
 
   describe.skip(WIN).skip(!HEADLESS)('signals', () => {
     // Cannot reliably send signals on Windows.
     it.slow()('should report browser close signal', async state => {
-      const result = await testSignal(state, (child, browserPid) => process.kill(browserPid), true);
-      expect(result.exitCode).toBe(0);
-      expect(result.browserExitCode).toBe('null');
-      expect(result.browserSignal).toBe('SIGTERM');
+      const wrapper = await setup(state);
+      const pid = await wrapper.out('pid');
+      process.kill(-pid, 'SIGTERM');
+      expect(await wrapper.out('exitCode')).toBe('null');
+      expect(await wrapper.out('signal')).toBe('SIGTERM');
+      process.kill(wrapper.child().pid);
+      await wrapper.childExitCode();
     });
     it.slow()('should report browser close signal 2', async state => {
-      const result = await testSignal(state, (child, browserPid) => process.kill(browserPid, 'SIGKILL'), true);
-      expect(result.exitCode).toBe(0);
-      expect(result.browserExitCode).toBe('null');
-      expect(result.browserSignal).toBe('SIGKILL');
+      const wrapper = await setup(state);
+      const pid = await wrapper.out('pid');
+      process.kill(-pid, 'SIGKILL');
+      expect(await wrapper.out('exitCode')).toBe('null');
+      expect(await wrapper.out('signal')).toBe('SIGKILL');
+      process.kill(wrapper.child().pid);
+      await wrapper.childExitCode();
     });
     it.slow()('should close the browser on SIGINT', async state => {
-      const result = await testSignal(state, child => process.kill(child.pid, 'SIGINT'));
-      expect(result.exitCode).toBe(130);
-      expect(result.browserExitCode).toBe('0');
-      expect(result.browserSignal).toBe('null');
+      const wrapper = await setup(state);
+      process.kill(wrapper.child().pid, 'SIGINT');
+      expect(await wrapper.out('exitCode')).toBe('0');
+      expect(await wrapper.out('signal')).toBe('null');
+      expect(await wrapper.childExitCode()).toBe(130);
     });
     it.slow()('should close the browser on SIGTERM', async state => {
-      const result = await testSignal(state, child => process.kill(child.pid, 'SIGTERM'));
-      expect(result.exitCode).toBe(0);
-      expect(result.browserExitCode).toBe('0');
-      expect(result.browserSignal).toBe('null');
+      const wrapper = await setup(state);
+      process.kill(wrapper.child().pid, 'SIGTERM');
+      expect(await wrapper.out('exitCode')).toBe('0');
+      expect(await wrapper.out('signal')).toBe('null');
+      expect(await wrapper.childExitCode()).toBe(0);
     });
     it.slow()('should close the browser on SIGHUP', async state => {
-      const result = await testSignal(state, child => process.kill(child.pid, 'SIGHUP'));
-      expect(result.exitCode).toBe(0);
-      expect(result.browserExitCode).toBe('0');
-      expect(result.browserSignal).toBe('null');
+      const wrapper = await setup(state);
+      process.kill(wrapper.child().pid, 'SIGHUP');
+      expect(await wrapper.out('exitCode')).toBe('0');
+      expect(await wrapper.out('signal')).toBe('null');
+      expect(await wrapper.childExitCode()).toBe(0);
     });
     it.slow()('should kill the browser on double SIGINT', async state => {
-      const result = await testSignal(state, child => {
-        process.kill(child.pid, 'SIGINT');
-        process.kill(child.pid, 'SIGINT');
-      });
-      expect(result.exitCode).toBe(130);
-      // TODO: ideally, we would expect the SIGKILL on the browser from
-      // force kill, but that's racy with sending two signals.
+      const wrapper = await setup(state, { stallOnClose: true });
+      process.kill(wrapper.child().pid, 'SIGINT');
+      await wrapper.out('stalled');
+      process.kill(wrapper.child().pid, 'SIGINT');
+      expect(await wrapper.out('exitCode')).toBe('null');
+      expect(await wrapper.out('signal')).toBe('SIGKILL');
+      expect(await wrapper.childExitCode()).toBe(130);
     });
-    // TODO: flaky - https://app.circleci.com/pipelines/github/microsoft/playwright/582/workflows/b49033ce-fe20-4029-b665-13fb331f842e/jobs/579
-    it.slow().fail(FFOX && LINUX)('should kill the browser on SIGINT + SIGTERM', async state => {
-      const result = await testSignal(state, child => {
-        process.kill(child.pid, 'SIGINT');
-        process.kill(child.pid, 'SIGTERM');
-      });
-      expect(result.exitCode).toBe(130);
-      // TODO: ideally, we would expect the SIGKILL on the browser from
-      // force kill, but that's racy with sending two signals.
+    it.slow()('should kill the browser on SIGINT + SIGTERM', async state => {
+      const wrapper = await setup(state, { stallOnClose: true });
+      process.kill(wrapper.child().pid, 'SIGINT');
+      await wrapper.out('stalled');
+      process.kill(wrapper.child().pid, 'SIGTERM');
+      expect(await wrapper.out('exitCode')).toBe('null');
+      expect(await wrapper.out('signal')).toBe('SIGKILL');
+      expect(await wrapper.childExitCode()).toBe(0);
     });
-    // TODO: flaky!
-    // - firefox: https://github.com/microsoft/playwright/pull/1911/checks?check_run_id=607148951
-    // - chromium: https://travis-ci.com/github/microsoft/playwright/builds/161356178
-    it.slow().fail((FFOX || CHROMIUM) && LINUX)('should kill the browser on SIGTERM + SIGINT', async state => {
-      const result = await testSignal(state, child => {
-        process.kill(child.pid, 'SIGTERM');
-        process.kill(child.pid, 'SIGINT');
-      });
-      expect(result.exitCode).toBe(130);
-      // TODO: ideally, we would expect the SIGKILL on the browser from
-      // force kill, but that's racy with sending two signals.
+    it.slow()('should kill the browser on SIGTERM + SIGINT', async state => {
+      const wrapper = await setup(state, { stallOnClose: true });
+      process.kill(wrapper.child().pid, 'SIGTERM');
+      await wrapper.out('stalled');
+      process.kill(wrapper.child().pid, 'SIGINT');
+      expect(await wrapper.out('exitCode')).toBe('null');
+      expect(await wrapper.out('signal')).toBe('SIGKILL');
+      expect(await wrapper.childExitCode()).toBe(130);
     });
   });
 });
diff --git a/test/fixtures/closeme.js b/test/fixtures/closeme.js
@@ -1,11 +1,16 @@
 (async() => {
-  const [, , playwrightIndex, browserType, options, exitOnClose] = process.argv;
-  const browserServer = await require(playwrightIndex)[browserType].launchServer(JSON.parse(options));
+  const { playwrightFile, browserTypeName, launchOptions, stallOnClose } = JSON.parse(process.argv[2]);
+  if (stallOnClose) {
+    launchOptions.__testHookGracefullyClose = () => {
+      console.log(`(stalled=>true)`);
+      return new Promise(() => {});
+    };
+  }
+  const browserServer = await require(playwrightFile)[browserTypeName].launchServer(launchOptions);
   browserServer.on('close', (exitCode, signal) => {
-    console.log(`browserClose:${exitCode}:${signal}:browserClose`);
-    if (exitOnClose)
-      process.exit(0);
+    console.log(`(exitCode=>${exitCode})`);
+    console.log(`(signal=>${signal})`);
   });
-  console.log(`browserPid:${browserServer.process().pid}:browserPid`);
-  console.log(`browserWS:${browserServer.wsEndpoint()}:browserWS`);
+  console.log(`(pid=>${browserServer.process().pid})`);
+  console.log(`(wsEndpoint=>${browserServer.wsEndpoint()})`);
 })();
