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

Add validation for Ember.onerror in testing. #304

Merged
merged 1 commit into from Dec 18, 2017
Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Dec 18, 2017

Ember.onerror intentionally is allowed to swallow errors, this is a very useful tool when in production to ensure that your application can be a tad bit more resilient to things like network issues and whatnot.

However, having an improperly configured Ember.onerror can lead to severe issues where your tests are passing but things in reality are completely broken.

This change leverages the validateErrorHandler utility function added in emberjs/ember-test-helpers#247 to automatically add a test that will fail if Ember.onerror does not rethrow the error it is provided. Importantly, this new test adding behavior is on by default (in order to help folks that are unaware they are in a broken state), but can easily be disabled (see below for details).

Example of Valid Ember.onerror

An example of a properly setup Ember.onerror is the following:

// app/app.js
import Ember from 'ember';

Ember.onerror = function(error) {
  if (Ember.testing) {
    throw error;
  }

  // ...desired development and production behaviors here...
};

Testing Production-like Behavior

In many cases, you will actually want to test that the swallowing does occur and the app is able to recover (e.g. in an acceptance test that intentionally rejects one of your network requests). To make that scenario easier to handle, we have created ember-test-friendly-error-handler which exposes a very simple API to allow specific tests to opt-in to the "production" behavior. That might look like:

// app/app.js
import Ember from 'ember';
import buildErrorHandler from 'ember-test-friendly-error-handler';

Ember.onerror = buildErrorHandler('Ember.onerror', (reason) => {
  reportErrorToService(reason);
  // whatever else you might want here...
});
// test/acceptance/something-test.js
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { visit } from '@ember/test-helpers';
import { 
  squelchErrorHandlerFor,
  unsquelchAllErrorHandlers
} from 'ember-test-friendly-error-handler';

module('some good description', function(hooks) {
  setupApplicationTest(hooks);

  hooks.afterEach(() => {
    unsquelchAllErrorHandlers();
  });

  test('network failure message is displayed', async function(assert) {
    squelchErrorHandlerFor('Ember.onerror');
    
    await visit('/some-route-here');
    
    // assert things here...
  });
});

Opting Out of Validation

In general, this is a bad idea. The validation exists specifically to protect applications from completely invalid test suites.

If however, you have good reason to disable validation you can opt-out by setting the setupEmberOnerrorValidation option when calling ember-qunit's start() method in tests/test-helper.js:

import Application from '../app';
import { setApplication } from '@ember/test-helpers';
import { start } from 'ember-qunit';

setApplication(Application.create({ autoboot: false }));

start({ setupEmberOnerrorValidation: false });

@rwjblue rwjblue force-pushed the validate-ember-onerror branch from 3450e62 to f80fcff Compare Dec 18, 2017
@rwjblue rwjblue merged commit ff69425 into master Dec 18, 2017
1 check passed
@rwjblue rwjblue deleted the validate-ember-onerror branch Dec 18, 2017
rwjblue added a commit to ember-cli/ember-cli-qunit that referenced this issue Jan 17, 2018
Main changes:

* [#307](emberjs/ember-qunit#307) Remove `exception` method from QUnit test adapter when using Ember 2.17+.  ([@rwjblue](https://github.com/rwjblue))
* [#306](emberjs/ember-qunit#306) Update to QUnit 2.5.0. ([@rwjblue](https://github.com/rwjblue))
  * Adds `assert.rejects`.
  * Reduces the delays around `assert.async` and returning promises from hook methods.
  * Adds a native promise unhandled rejection handler.
  * Fixes an issue with `notrycatch` mode being ignored for promises returned (e.g. it would previously always `.catch` every returned promise).
* [#304](emberjs/ember-qunit#304) Add validation for `Ember.onerror` in testing. ([@rwjblue](https://github.com/rwjblue))
alexlafroscia added a commit to alexlafroscia/ember-new-relic that referenced this issue Jul 13, 2018
Turbo87 pushed a commit to ember-cli/ember-cli-eslint that referenced this issue Oct 17, 2018
Bumps [ember-cli-qunit](https://github.com/ember-cli/ember-cli-qunit) from 4.0.1 to 4.4.0.
<details>
<summary>Changelog</summary>

*Sourced from [ember-cli-qunit's changelog](https://github.com/ember-cli/ember-cli-qunit/blob/master/CHANGELOG.md).*

> # Change Log
> 
> ## v4.3.2 (2018-02-16)
> 
> #### 🐛 Bug Fix
> * Update ember-qunit to 3.3.2 (fixing an issue with `waitUntil`)
> 
> #### Committers: 1
> - Robert Jackson ([rwjblue](https://github.com/rwjblue))
> 
> ## v4.3.1 (2018-02-11)
> 
> #### 🚀 Enhancement
> * [#204](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/pull/204) Update dependencies to latest versions. ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
> 
> #### 🏠 Internal
> * [#200](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/pull/200) Update RELEASE.md for lerna-changelog & travis deploy. ([[**kategengler**](https://github.com/kategengler)](https://github.com/kategengler))
> 
> #### Committers: 2
> - Katie Gengler ([kategengler](https://github.com/kategengler))
> - Robert Jackson ([rwjblue](https://github.com/rwjblue))
> 
> ## v4.3.0 (2018-01-17)
> 
> #### 🐛 Bug Fix
> * Update minimum version of ember-qunit to 3.3.0.
>   * [emberjs/ember-qunit#307](https://github-redirect.dependabot.com/emberjs/ember-qunit/pull/307) Remove `exception` method from QUnit test adapter when using Ember 2.17+.  ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
>   * [emberjs/ember-qunit#306](https://github-redirect.dependabot.com/emberjs/ember-qunit/pull/306) Update to QUnit 2.5.0. ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
>     * Adds `assert.rejects`.
>     * Reduces the delays around `assert.async` and returning promises from hook methods.
>     * Adds a native promise unhandled rejection handler.
>     * Fixes an issue with `notrycatch` mode being ignored for promises returned (e.g. it would previously always `.catch` every returned promise).
>   * [emberjs/ember-qunit#304](https://github-redirect.dependabot.com/emberjs/ember-qunit/pull/304) [BREAKING BUGFIX] Add validation for `Ember.onerror` in testing. ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
> 
> ## v4.2.1 (2017-12-18)
> 
> #### 🐛 Bug Fix
> * [#198](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/pull/198) Update minimum version of ember-qunit. ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
>   * Ensures re-exports from ember-test-helpers are still present (with a deprecation).
>   * Ensures that `Ember.testing` is set properly from "normal" non `moduleFor*` and/or `setup*Test` style tests.
> 
> #### 🏠 Internal
> * [#197](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/pull/197) Update ember-cli blueprint to 2.17.1.. ([[**rwjblue**](https://github.com/rwjblue)](https://github.com/rwjblue))
> 
> #### Committers: 1
> - Robert Jackson ([rwjblue](https://github.com/rwjblue))
> 
> ## v4.2.0 (2017-12-17)
> 
> #### 🚀 Enhancement
></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`4ad1af2`](ember-cli/ember-cli-qunit@4ad1af2) release v4.4.0 🎉
- [`f4bd36b`](ember-cli/ember-cli-qunit@f4bd36b) Merge pull request [#212](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/issues/212) from ember-cli/upgrade
- [`f32d88a`](ember-cli/ember-cli-qunit@f32d88a) bump ember-qunit
- [`63ac2f2`](ember-cli/ember-cli-qunit@63ac2f2) Merge pull request [#210](https://github-redirect.dependabot.com/ember-cli/ember-cli-qunit/issues/210) from samselikoff/upgrade-ember-qunit
- [`300b36c`](ember-cli/ember-cli-qunit@300b36c) Upgrade ember-qunit
- [`d3dc0f4`](ember-cli/ember-cli-qunit@d3dc0f4) v4.3.2
- [`f4897da`](ember-cli/ember-cli-qunit@f4897da) Add v4.3.2 to CHANGELOG.md.
- [`c3a2396`](ember-cli/ember-cli-qunit@c3a2396) Update ember-qunit to v3.3.2.
- [`ac4ea28`](ember-cli/ember-cli-qunit@ac4ea28) v4.3.1
- [`57b348a`](ember-cli/ember-cli-qunit@57b348a) Add v4.3.1 to CHANGELOG.md.
- Additional commits viewable in [compare view](ember-cli/ember-cli-qunit@v4.0.1...v4.4.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=ember-cli-qunit&package-manager=npm_and_yarn&previous-version=4.0.1&new-version=4.4.0)](https://dependabot.com/compatibility-score.html?dependency-name=ember-cli-qunit&package-manager=npm_and_yarn&previous-version=4.0.1&new-version=4.4.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

Dependabot will **not** automatically merge this PR because it includes a minor update to a development dependency.

---

**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Turbo87 added a commit to Turbo87/sentry-javascript that referenced this issue Oct 26, 2018
Turbo87 added a commit to Turbo87/sentry-javascript that referenced this issue Oct 30, 2018
kamilogorek pushed a commit to getsentry/sentry-javascript that referenced this issue Oct 31, 2018
@argote
Copy link

argote commented Dec 29, 2018

Is there a way to disable validation on a per-test basis? Seems this conflicts with using onerror to test for expected errors in helper rendering integration tests.
See https://discuss.emberjs.com/t/how-to-catch-errors-in-component-rendering-test/14854

module('Integration | Helper | my-helper', function(hooks) {
  setupRenderingTest(hooks);

  test('it throws error and renders nothing when null passed in', async function(assert) {
    this.set('inputValue', null);

    const done = assert.async();
    Ember.onerror = function(error) {
      assert.ok(error.message.includes('null passed in'));
      done();
    };

    await render(hbs`{{my-helper inputValue}}`);

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

@rwjblue
Copy link
Member Author

rwjblue commented Dec 29, 2018

You should add an afterEach to reset Ember.onerror, otherwise all of your other tests will be doing that assertion (and swallowing errors) whenever an error happens. This scenario is exactly the kind of thing we are trying to catch with this assertion.

@argote
Copy link

argote commented Dec 29, 2018

Thanks! That's actually what I ended up doing, however I was not aware that the Ember instance is shared across all tests. I was expecting each test to be atomic and reset all Ember values to the default.

@ppcano
Copy link

ppcano commented Feb 20, 2019

If the following test is executed at the end of our test suite, Ember.onerror will be reset and the ember-qunit: Ember.onerror validation: Ember.onerror is functioning properly test fails.

import { test, module } from 'qunit';
import { setupTest } from 'ember-qunit';

module('xxx', function(hooks) {
  setupTest(hooks);

  test('', function() {
    ...
  });

});

Perhaps, there is an expected configuration in our app or test suite. I am asking for the recommended way to fix this situation; I ended up using a custom setupForTest which is a setupTest wrapper:

import Ember from 'ember';
import { setupTest } from 'ember-qunit';

const setupForTest = function(hooks) {
  setupTest(hooks);

  hooks.beforeEach(function() {
    Ember.onerror = function(error) {
      throw error;
    };
  });

};

export default setupForTest;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants