More robust handling in setup-onerror

Resolves an issue where Ember.onerror could get cached too soon.

Fixes #993

Author: wagenet

addon-test-support/@ember/test-helpers/setup-context.ts

@@ -5,6 +5,7 @@ import { setOwner } from '@ember/application';
 
 import buildOwner, { Owner } from './build-owner';
 import { _setupAJAXHooks, _teardownAJAXHooks } from './settled';
+import { _prepareOnerror } from './setup-onerror';
 import Ember from 'ember';
 import { assert } from '@ember/debug';
 import global from './global';
@@ -180,6 +181,8 @@ export default function setupContext(
 
   registerDestructor(context, cleanup);
 
+  _prepareOnerror(context);
+
   return Promise.resolve()
     .then(() => {
       let application = getApplication();

addon-test-support/@ember/test-helpers/setup-onerror.ts

@@ -1,6 +1,7 @@
 import Ember from 'ember';
+import { BaseContext, getContext } from './setup-context';
 
-const ORIGINAL_EMBER_ONERROR: (error: Error) => void | undefined = Ember.onerror;
+let cachedOnerror: Map<BaseContext, ((error: Error) => void) | undefined> = new Map();
 
 /**
  * Sets the `Ember.onerror` function for tests. This value is intended to be reset after
@@ -21,10 +22,24 @@ const ORIGINAL_EMBER_ONERROR: (error: Error) => void | undefined = Ember.onerror
  * });
  */
 export default function setupOnerror(onError?: (error: Error) => void): void {
+  let context = getContext();
+
+  if (!context) {
+    throw new Error('Must setup test context before calling setupOnerror');
+  }
+
+  if (!cachedOnerror.has(context)) {
+    throw new Error(
+      '_cacheOriginalOnerror must be called before setupOnerror. Normally, this will happen as part of your test harness.'
+    );
+  }
+
   if (typeof onError !== 'function') {
-    onError = ORIGINAL_EMBER_ONERROR;
+    onError = cachedOnerror.get(context);
   }
 
+  // @ts-ignore types are incorrect and don't allow undefined.
+  // See https://github.com/DefinitelyTyped/DefinitelyTyped/pull/51383
   Ember.onerror = onError;
 }
 
@@ -42,3 +57,29 @@ export default function setupOnerror(onError?: (error: Error) => void): void {
  * })
  */
 export const resetOnerror: Function = setupOnerror;
+
+/**
+ * Caches the current value of Ember.onerror. When `setupOnerror` is called without a value
+ * or when `resetOnerror` is called the value will be set to what was cached here.
+ *
+ * @private
+ * @param {BaseContext} context the text context
+ */
+export function _prepareOnerror(context: BaseContext) {
+  if (cachedOnerror.has(context)) {
+    throw new Error('_prepareOnerror should only be called once per-context');
+  }
+
+  cachedOnerror.set(context, Ember.onerror);
+}
+
+/**
+ * Removes the cached value of Ember.onerror.
+ *
+ * @private
+ * @param {BaseContext} context the text context
+ */
+export function _cleanupOnerror(context: BaseContext) {
+  resetOnerror();
+  cachedOnerror.delete(context);
+}

addon-test-support/@ember/test-helpers/teardown-context.ts

@@ -1,6 +1,7 @@
 import { TestContext } from './setup-context';
 import { Promise } from './-utils';
 import settled from './settled';
+import { _cleanupOnerror } from './setup-onerror';
 import { destroy } from '@ember/destroyable';
 
 /**
@@ -29,6 +30,8 @@ export default function teardownContext(
 
   return Promise.resolve()
     .then(() => {
+      _cleanupOnerror(context);
+
       destroy(context);
     })
     .finally(() => {

tests/unit/setup-ember-onerror-test.js

@@ -1,46 +1,67 @@
 import Ember from 'ember';
 import { module, test } from 'qunit';
 import hasEmberVersion from '@ember/test-helpers/has-ember-version';
+import { setupContext, teardownContext } from '@ember/test-helpers';
 import { setupOnerror, resetOnerror } from '@ember/test-helpers';
 
 module('setupOnerror', function (hooks) {
-  hooks.afterEach(function () {
-    resetOnerror();
+  let context;
+
+  hooks.beforeEach(function () {
+    context = {};
+  });
+
+  hooks.afterEach(async function () {
+    if (context.owner) {
+      await teardownContext(context);
+    }
   });
 
   if (hasEmberVersion(2, 4)) {
-    test('Ember.onerror is undefined by default', function (assert) {
-      assert.expect(1);
+    module('with context set', function (hooks) {
+      hooks.beforeEach(async function () {
+        await setupContext(context);
+      });
 
-      assert.equal(Ember.onerror, undefined);
-    });
+      test('Ember.onerror is undefined by default', function (assert) {
+        assert.expect(1);
 
-    test('Ember.onerror is setup correctly', async function (assert) {
-      assert.expect(2);
+        assert.equal(Ember.onerror, undefined);
+      });
 
-      let onerror = err => err;
+      test('Ember.onerror is setup correctly', async function (assert) {
+        assert.expect(2);
 
-      assert.equal(Ember.onerror, undefined);
+        let onerror = err => err;
 
-      setupOnerror(onerror);
+        assert.equal(Ember.onerror, undefined);
 
-      assert.equal(Ember.onerror, onerror);
-    });
+        setupOnerror(onerror);
+
+        assert.equal(Ember.onerror, onerror);
+      });
 
-    test('Ember.onerror is reset correctly', async function (assert) {
-      assert.expect(3);
+      test('Ember.onerror is reset correctly', async function (assert) {
+        assert.expect(3);
 
-      let onerror = err => err;
+        let onerror = err => err;
 
-      assert.equal(Ember.onerror, undefined);
+        assert.equal(Ember.onerror, undefined);
 
-      setupOnerror(onerror);
+        setupOnerror(onerror);
 
-      assert.equal(Ember.onerror, onerror);
+        assert.equal(Ember.onerror, onerror);
 
-      resetOnerror();
+        resetOnerror();
+
+        assert.equal(Ember.onerror, undefined);
+      });
+    });
 
-      assert.equal(Ember.onerror, undefined);
+    test('it raises an error without context', function (assert) {
+      assert.throws(() => {
+        setupOnerror();
+      }, /Must setup test context before calling setupOnerror/);
     });
   }
 });