Handle transient browser disconnections

lib/commands/exam.js

@@ -325,24 +325,16 @@ module.exports = TestCommand.extend({
       );
     };
 
+    // Called for processError, processExit, and the finish() wrapper below.
+    // Flag avoids double-counting.
     const browserTerminationHandler = function () {
-      // browserTerminationHandler is called for disconnect, processError or processExit events.
-      // disconnect and processExit events is fired during global error and successful test runs.
-      // On successful test runs, browserExitHandler should already be called. And is unnecessary
-      // to call it again, so we should return. This is covered by this.finish = true
-      // On global failure cases, it's possible that this.finish is also true. So we must check
-      // the timers set by onProcessExit
-      // https://github.com/testem/testem/blob/master/lib/runners/browser_test_runner.js#L266
-      // or onProcessError in testem.
-      // https://github.com/testem/testem/blob/master/lib/runners/browser_test_runner.js#L252
-      // If either timers is set, we should record the failed browser and call browserExitHandler
-      if (this.finished && !this.onProcessExitTimer && !this.pendingTimer) {
+      if (this._emberExamExitRecorded) {
         return;
       }
+      this._emberExamExitRecorded = true;
       if (commands.get('writeExecutionFile')) {
         testemEvents.recordFailedBrowserId(this.launcher, ui);
       }
-
       browserExitHandler.call(this, true);
     };
 
@@ -381,16 +373,31 @@ module.exports = TestCommand.extend({
         init = true;
       }
 
+      // Monkey-patch finish() — testem exposes no event for "browser truly
+      // done", and finish() is the one method all termination paths flow
+      // through (clean exit, disconnect timeout, processExit timer). The
+      // socket `disconnect` event fires on transient drops too, so it
+      // can't be used. Microtask defers past after-tests-complete.
+      if (!this._emberExamFinishHooked) {
+        this._emberExamFinishHooked = true;
+        const originalFinish = this.finish;
+        const runner = this;
+        this.finish = function () {
+          const result = originalFinish.apply(this, arguments);
+          Promise.resolve().then(() => browserTerminationHandler.call(runner));
+          return result;
+        };
+      }
+
       if (typeof this.launcher !== 'undefined' && this.launcher !== null) {
         testemEvents.recordStartedLauncherId(this.launcher.id);
       }
     };
 
-    events['after-tests-complete'] = browserExitHandler;
-
-    events['disconnect'] = function () {
-      // To prevent handling exiting browser browser disconnects from errors `disconnect` callback's needed to be registered.
-      browserTerminationHandler.bind(this)();
+    events['after-tests-complete'] = function (...args) {
+      // Mark the runner recorded; the finish() wrapper above checks this flag.
+      this._emberExamExitRecorded = true;
+      return browserExitHandler.apply(this, args);
     };
 
     events['testem:test-done-metadata'] = (details) => {

node-tests/unit/commands/exam-test.js

@@ -152,4 +152,117 @@ describe('ExamCommand', function () {
       });
     });
   });
+
+  describe('browser socket events', function () {
+    function createRunner(launcherId) {
+      return {
+        finished: false,
+        launcher: {
+          id: launcherId,
+          settings: { test_page: `browser=${launcherId}` },
+        },
+        finish() {
+          if (this.finished) return;
+          this.finished = true;
+        },
+      };
+    }
+
+    async function flushMicrotasks() {
+      await Promise.resolve();
+      await Promise.resolve();
+    }
+
+    beforeEach(function () {
+      this.command = createCommand();
+      this.command.commands = new Map([
+        ['loadBalance', false],
+        ['writeExecutionFile', false],
+        ['writeModuleMetadataFile', false],
+      ]);
+      this.events = this.command._setupAndGetBrowserSocketEvents({
+        testPage: ['page=1', 'page=2'],
+      });
+    });
+
+    it('counts a browser once on after-tests-complete', function () {
+      const runner = createRunner(1);
+      this.events['tests-start'].call(runner);
+      this.events['after-tests-complete'].call(runner);
+
+      assert.strictEqual(
+        this.command.testemEvents.stateManager.getCompletedBrowser(),
+        1,
+      );
+    });
+
+    it('does not double-count when finish() runs after after-tests-complete', async function () {
+      const runner = createRunner(1);
+      this.events['tests-start'].call(runner);
+      this.events['after-tests-complete'].call(runner);
+      runner.finish();
+      await flushMicrotasks();
+
+      assert.strictEqual(
+        this.command.testemEvents.stateManager.getCompletedBrowser(),
+        1,
+      );
+    });
+
+    it('counts a browser when finish() fires without after-tests-complete', async function () {
+      const runner = createRunner(1);
+      this.events['tests-start'].call(runner);
+      runner.finish();
+      await flushMicrotasks();
+
+      assert.strictEqual(
+        this.command.testemEvents.stateManager.getCompletedBrowser(),
+        1,
+      );
+    });
+
+    it('counts only once across multiple finish() calls', async function () {
+      const runner = createRunner(1);
+      this.events['tests-start'].call(runner);
+      runner.finish();
+      await flushMicrotasks();
+      runner.finish();
+      await flushMicrotasks();
+
+      assert.strictEqual(
+        this.command.testemEvents.stateManager.getCompletedBrowser(),
+        1,
+      );
+    });
+
+    it('does not re-wrap finish on repeat tests-start', function () {
+      const runner = createRunner(1);
+      this.events['tests-start'].call(runner);
+      const wrapped = runner.finish;
+      this.events['tests-start'].call(runner);
+
+      assert.strictEqual(runner.finish, wrapped);
+    });
+
+    it('registers no disconnect handler', function () {
+      assert.strictEqual(this.events['disconnect'], undefined);
+    });
+
+    it('does not reset the module queue when one of two browsers finishes', async function () {
+      const runner1 = createRunner(1);
+      const runner2 = createRunner(2);
+      this.events['tests-start'].call(runner1);
+      this.events['tests-start'].call(runner2);
+      this.command.testemEvents.stateManager.setTestModuleQueue(['foo', 'bar']);
+
+      this.events['after-tests-complete'].call(runner2);
+      runner2.finish();
+      await flushMicrotasks();
+
+      assert.deepEqual(
+        this.command.testemEvents.stateManager.getTestModuleQueue(),
+        ['foo', 'bar'],
+      );
+    });
+  });
 });