[BREAKING?] fix(settled, waitUntil): reject with error thrown in run loop

[buschtoens]

This PR is currently still WIP and failing some tests, but I'd like to receive feedback on the implementation and course-correct, where necessary.

---

The goal of this PR is to fix the longstanding issue of test helpers not propagating run loop errors thrown during their execution, but instead causing an out of band "global failure" that fails the test. For example:

test('`render` rejects with error thrown inside the run loop', async function (assert) {
  // 🔥 BUG: `render` doesn't reject, instead we get an uncatchable "global failure".
  await assert.rejects(
    render(hbs`<UnknownComponent />`),
    /unknown-component/
  );
});

test('`click` rejects with run loop errors', async function (assert) {
  this.owner.register('component:click-test', Component.extend({
    layout: hbs`<div class="click-test"></div>`,

    click() {
      later(() => {
        throw new Error('bazinga');
      }, 10);
    },
  }));

  await render(hbs`<ClickTest />`);

  // 🔥 BUG: `click` doesn't reject, instead we get an uncatchable "global failure".
  await assert.rejects(
    click('.click-test'),
    /bazinga/,
    'rejects with the error thrown inside the run loop'
  );
});

Fixes #310. Fixes #453. Relates to #440, #1128, ember-qunit#592

---

This PR adds an rejectOnError option to waitUntil. It defaults to true, if the test context is set up correctly for use with setupOnerror.

If enabled, waitUntil will reject when an error propagates to Ember.onerror, using setupOnerror under the hood. As such, (a/sync) errors thrown in a run loop will cause waitUntil to fail.

Almost all test helpers end with a call to settled, which just calls waitUntil. Therefore they'll all automatically benefit from this change and reject, if an error is thrown during their execution.

I've written down an in-depth analysis of the flow in this #1128 comment.

[buschtoens]

I'm not really fond of this, because:

it will override any error listeners you might have set up before.

@default true if the test context has been setup for usage with setupOnerror

This limitation arises from how setupOnerror works internally. Alternatively, we could always instrument Ember.onerror in tests, somewhat like:

const originalOnError = Ember.onerror;
let onErrorOverride?: typeof Ember.onerror;

Ember.onerror = error => {
  // _Always_ let `waitUntil` know about the error.
  notifyInternalOnError(error);

  // If the user called `setupOnerror(...)`, call the override.
  if (onErrorOverride)
    return onErrorOverride(error);
  
  // Otherwise call the original `Ember.onerror`.
  return originalOnError?.(error);
};

export function setupOnerror(onError?: typeof Ember.onerror): 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 = cachedOnerror.get(context);
  }

  onErrorOverride = onError;
}

export function resetOnerror(): void {
  let context = getContext();
  if (context && cachedOnerror.has(context)) {
    onErrorOverride = cachedOnerror.get(context);
  }
}

[buschtoens]

Also, letting rejectOnError default to true will inevitably break a lot of existing test suites, that currently try to emulate this missing feature by using setupOnerror or manually hooking into Ember.onerror.

However, I believe that ultimately the behavior implemented in this PR is what a user would expect. So while this breaking change may be a pain, because it will require changing all tests dealing with such errors, I think that it's a very welcome change.

Maybe we could even conjur up a codemod to ease the transition. Or we could add a compatibility mode, that will essentially change the order of notifyInternalOnError and onErrorOverride, so that "old" tests just continue to work.

const originalOnError = Ember.onerror;
let onErrorOverride?: typeof Ember.onerror;
let preferOnErrorOverride = true;

Ember.onerror = error => {
  // Compatibility mode for old tests.
  if (preferOnErrorOverride && onErrorOverride)
    return onErrorOverride(error);

  // _Always_ let `waitUntil` know about the error.
  notifyInternalOnError(error);

  // If the user called `setupOnerror(...)`, call the override.
  if (onErrorOverride)
    return onErrorOverride(error);
  
  // Otherwise call the original `Ember.onerror`.
  return originalOnError?.(error);
};

We should also consider adding a setter trap for Ember.onerror to catch users manually overriding it in tests.

A second next argument provided to onErrorOverride, like with registerWarnHandler would also be nice:

const originalOnError = Ember.onerror;
let onErrorOverride?: typeof Ember.onerror;

function next() {
  notifyInternalOnError(error);
  return originalOnError?.(error);
}

Ember.onerror = error => {
  if (onErrorOverride)
    return onErrorOverride(error, next);

  return next();
};

[ef4]

This seems good. I think it's the best solution that doesn't require changes to ember itself.

But I view this as a workaround for a feature ember itself should offer. There should be public API that lets you implement render, and it should include more specific error handling than a single global callback.

[jkeen]

Is there any movement on this, or perhaps an alternative available pattern?

I'm currently using this asyncThrows helper via ember-custom-assertions (https://gist.github.com/samselikoff/ad5e3695383b91599ee428bf9a2d22ca) but in trying to upgrade some apps, importing Ember is no longer allowed so it's not working.

It'd be great to get some official pattern for handling testing async render errors!

[NullVoxPopuli]

Should we pursue finishing / deciding what to do with this PR?