Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
9 changes: 9 additions & 0 deletions API.md
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,7 @@ while *not* settled (e.g. "loading" or "pending" states).

* `options.timeout` **[number][67]** the maximum amount of time to wait (optional, default `1000`)
* `options.timeoutMessage` **[string][62]** the message to use in the reject on timeout (optional, default `'waitUntil timed out'`)
* `options.rejectOnError` **[boolean][69]** reject when an operation in a run loop has failed (optional, default `true`, if the test context has been setup for usage with [`setupOnerror`][49])

#### Examples

Expand All @@ -588,6 +589,14 @@ a definition of "settled state").

Returns **[Promise][64]\<void>** resolves when settled

#### Parameters

* `options` **[Object][70]?** options passed to [`waitUntil`][26] (optional, default `{}`)

* `options.timeout` **[number][67]** the maximum amount of time to wait (optional, default `Infinity`)
* `options.timeoutMessage` **[string][62]** the message to use in the reject on timeout (optional, default `'settled timed out'`)
* `options.rejectOnError` **[boolean][69]** reject when an operation in a run loop has failed (optional, default `true`, if the test context has been setup for usage with [`setupOnerror`][49])

### isSettled

Checks various settledness metrics (via `getSettledState()`) to determine if things are settled or not.
Expand Down
18 changes: 15 additions & 3 deletions addon-test-support/@ember/test-helpers/settled.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import Ember from 'ember';
import EmberApplicationInstance from '@ember/application/instance';

import { nextTick } from './-utils';
import waitUntil from './wait-until';
import waitUntil, { Options } from './wait-until';
// @ts-ignore Referenced in DocBlock.
import type { setupOnerror } from './setup-onerror';
import { hasPendingTransitions } from './setup-application-context';
import { hasPendingWaiters } from '@ember/test-waiters';
import DebugInfo, { TestDebugInfo } from './-internal/debug-info';
Expand Down Expand Up @@ -281,8 +283,18 @@ export function isSettled(): boolean {
a definition of "settled state").

@public
@param {Object} [options] options passed to {@link waitUntil}
@param {number} [options.timeout=Infinity] the maximum amount of time to wait
@param {string} [options.timeoutMessage='settled timed out'] the message to
use in the reject on timeout
@param {boolean} [options.rejectOnError] reject when an operation in a run
loop has failed; defaults to `true`, if the test context has been setup for usage with {@link setupOnerror}
@returns {Promise<void>} resolves when settled
*/
export default function settled(): Promise<void> {
return waitUntil(isSettled, { timeout: Infinity }).then(() => {});
export default function settled(options?: Options): Promise<void> {
return waitUntil(isSettled, {
timeout: Infinity,
timeoutMessage: 'settled timed out',
...options,
}).then(() => {});
}
33 changes: 28 additions & 5 deletions addon-test-support/@ember/test-helpers/setup-onerror.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,46 @@ let cachedOnerror: Map<BaseContext, ((error: Error) => void) | undefined> =
* });
*/
export default function setupOnerror(onError?: (error: Error) => void): void {
if (typeof onError !== 'function') {
onError = cachedOnerror.get(_getContextForOnError(true));
}

Ember.onerror = onError;
}

/**
* If setup correctly, returns the test context that will be used for {@link setupOnerror}. If `throwIfNotSetup` is true, throws an error, if the test context is not setup correctly.
*
* @private
* @param {Boolean} [throwIfNotSetup=false] if `true`, will throw an error instead of returning `undefined`, if the test context has not been setup for {@link setupOnerror}
* @returns {BaseContext | undefined} the test context that will be used for{@link setupOnerror}, if setup correctly
* @throws {Error} if test context has not been setup for {@link setupOnerror}
*/
export function _getContextForOnError(
throwIfNotSetup: true
): BaseContext | never;
export function _getContextForOnError(
throwIfNotSetup?: false
): BaseContext | undefined;
// eslint-disable-next-line require-jsdoc
export function _getContextForOnError(
throwIfNotSetup = false
): BaseContext | undefined | never {
let context = getContext();

if (!context) {
if (!throwIfNotSetup) return;
throw new Error('Must setup test context before calling setupOnerror');
}

if (!cachedOnerror.has(context)) {
if (!throwIfNotSetup) return;
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);
}

Ember.onerror = onError;
return context;
}

/**
Expand Down
44 changes: 36 additions & 8 deletions addon-test-support/@ember/test-helpers/wait-until.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { futureTick, Promise } from './-utils';
import setupOnerror, {
resetOnerror,
_getContextForOnError,
} from './setup-onerror';

const TIMEOUTS = [0, 1, 2, 5, 7];
const MAX_TIMEOUT = 10;
Expand All @@ -8,6 +12,17 @@ type Falsy = false | 0 | '' | null | undefined;
export interface Options {
timeout?: number;
timeoutMessage?: string;

/**
* Instrument `Ember.onerror` and reject, when it is called. This is useful
* for detecting that an operation inside a run loop has failed.
*
* This uses {@link setupOnerror}, so it will override any error listeners you
* might have set up before.
*
* @default true if the test context has been setup for usage with {@link setupOnerror}
*/
rejectOnError?: boolean;
Comment on lines +16 to +25
Copy link
Contributor Author

@buschtoens buschtoens Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  }
}

Copy link
Contributor Author

@buschtoens buschtoens Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
};

}

/**
Expand All @@ -21,6 +36,7 @@ export interface Options {
@param {Object} [options] options used to override defaults
@param {number} [options.timeout=1000] the maximum amount of time to wait
@param {string} [options.timeoutMessage='waitUntil timed out'] the message to use in the reject on timeout
@param {boolean} [options.rejectOnError] reject when an operation in a run loop has failed; defaults to `true`, if the test context has been setup for usage with {@link setupOnerror}
@returns {Promise} resolves with the callback value when it returns a truthy value

@example
Expand All @@ -33,19 +49,24 @@ export interface Options {
*/
export default function waitUntil<T>(
callback: () => T | void | Falsy,
options: Options = {}
{
timeout = 1000,
timeoutMessage = 'waitUntil timed out',
rejectOnError = Boolean(_getContextForOnError(false)),
}: Options = {}
): Promise<T> {
let timeout = 'timeout' in options ? (options.timeout as number) : 1000;
let timeoutMessage =
'timeoutMessage' in options
? options.timeoutMessage
: 'waitUntil timed out';

// creating this error eagerly so it has the proper invocation stack
let waitUntilTimedOut = new Error(timeoutMessage);

return new Promise(function (resolve, reject) {
let time = 0;
let error: unknown = undefined;

if (rejectOnError) {
setupOnerror((e) => {
error = e;
});
}

// eslint-disable-next-line require-jsdoc
function scheduleCheck(timeoutsIndex: number) {
Expand All @@ -65,13 +86,20 @@ export default function waitUntil<T>(
return;
}

if (typeof error !== 'undefined') {
resetOnerror();
reject(error);
return;
}

if (value) {
if (rejectOnError) resetOnerror();
resolve(value);
} else if (time < timeout) {
scheduleCheck(timeoutsIndex + 1);
} else {
if (rejectOnError) resetOnerror();
reject(waitUntilTimedOut);
return;
}
}, interval);
}
Expand Down
31 changes: 30 additions & 1 deletion tests/integration/settled-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getSettledState,
render,
} from '@ember/test-helpers';
import getElement from '@ember/test-helpers/dom/-get-element';
import hasEmberVersion from '@ember/test-helpers/has-ember-version';
import { module, test } from 'qunit';
import { hbs } from 'ember-cli-htmlbars';
Expand Down Expand Up @@ -117,6 +118,16 @@ const TestComponent5 = Component.extend({
},
});

const TestComponent6 = Component.extend({
layout: hbs`<div class="test-component"></div>`,

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

module('settled real-world scenarios', function (hooks) {
if (!hasEmberVersion(2, 4)) {
return;
Expand Down Expand Up @@ -166,7 +177,6 @@ module('settled real-world scenarios', function (hooks) {
test('does not error for various argument types', async function (assert) {
assert.expect(0); // no assertions, just shouldn't error

await settled(3000);
await settled(null);
await settled(undefined);
await settled();
Expand Down Expand Up @@ -216,4 +226,23 @@ module('settled real-world scenarios', function (hooks) {

assert.equal(this.element.textContent, 'async value');
});

test('it rejects with run loop errors', async function (assert) {
this.owner.register('component:x-test-6', TestComponent6);

await render(hbs`{{x-test-6}}`);

// Run in next tick, so that the test progresses to the assertion.
setTimeout(() => {
// Intentionally not using the `click` helper, because it uses `settled`
// internally.
getElement('.test-component').click();
});

await assert.rejects(
settled({ rejectOnError: true }),
/bazinga/,
'rejects with the error thrown inside the run loop'
);
});
});
15 changes: 15 additions & 0 deletions tests/unit/dom/click-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
teardownContext,
_registerHook,
} from '@ember/test-helpers';
import { later } from '@ember/runloop';
import {
buildInstrumentedElement,
instrumentElement,
Expand Down Expand Up @@ -60,6 +61,20 @@ module('DOM Helper: click', function (hooks) {
}
});

test('rejects with error thrown inside the run loop', async function (assert) {
element = document.createElement('div');
insertElement(element);

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

await setupContext(context);
await assert.rejects(click(element), /bazinga/);
});

module('non-focusable element types', function () {
test('clicking a div via selector with context set', async function (assert) {
element = buildInstrumentedElement('div');
Expand Down
7 changes: 7 additions & 0 deletions tests/unit/setup-rendering-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ module('setupRenderingContext', function (hooks) {
assert.ok(isSettled(), 'should be settled');
});

test('`render` rejects with error thrown inside the run loop', async function (assert) {
await assert.rejects(
render(hbs`<UnknownComponent />`),
/unknown-component/
);
});

overwriteTest('element');

test('it sets up test metadata', function (assert) {
Expand Down
19 changes: 18 additions & 1 deletion tests/unit/wait-until-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { module, test } from 'qunit';
import { Promise } from 'rsvp';
import { waitUntil } from '@ember/test-helpers';
import { waitUntil, setupContext, teardownContext } from '@ember/test-helpers';
import { later } from '@ember/runloop';

module('DOM helper: waitUntil', function () {
test('waits until the provided function returns true', async function (assert) {
Expand Down Expand Up @@ -89,4 +90,20 @@ module('DOM helper: waitUntil', function () {

await new Promise((resolve) => setTimeout(resolve, 100));
});

test('rejects when run loop throws', async function (assert) {
const context = await setupContext({});

later(() => {
throw new Error('error goes here');
}, 10);

await assert.rejects(
waitUntil(() => false, { rejectOnError: true }),
/error goes here/,
'valid error was thrown'
);

await teardownContext(context);
});
});