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

Test.adapter.exception forces acceptance test to fail (RSVP error fails test) #12791

Closed
pixelhandler opened this Issue Jan 9, 2016 · 19 comments

Comments

Projects
None yet
@pixelhandler
Copy link
Contributor

pixelhandler commented Jan 9, 2016

My workaround is here: emberjs/guides#1157 at first I thought it may be best to address with an update to testing guides, but not so sure that is the best way to fix.

Steps to reproduce:

Here is an example repository with an Ember CLI app: https://github.com/pixelhandler/application-error-test
(Can't be done w/ a jsbin/fiddle/ember-twiddle - since this issue is with the testing env.)

Below are the steps I used to create the example app.

  1. Create an app with one of the model hooks making an XHR request.
  2. Create an application-error template, perhaps use {{model.message}} since the error will become the model via application-error route's setupController hook.
  3. Create an acceptance test to test that the application-error template displays the error message.
  4. In the acceptance test, mock the XHR - to return a 502 response. (I used Sinon.js with ember-sinon addon)
  5. Write an assertion that checks the DOM, e.g. visit('/'); andThen(function(){ assert.equal(find('.error-message').length(), 1); });

Expected Result:

The test passes even though an error was thrown in the route's model hook by the mocked 502 response.

Actual Result:

The Assertion passes as the template is rendered, but the test fails since Test.adapter.exception is called. Which throws, and causes the test to fail - as a result of the "same" expected error that causes the application-error substate to render.

Attempts to solve:

Because an error is thrown and the env is testing - Ember RSVP calls Test.adapter.exception. Which fails the test; because there was an error (we expected an error to be handled by the application-error route/controller/template and it was). In the acceptance test, using assert.throws will not help since it's async.

I found that I had to reopen my test adapter in the acceptance test like so:

tests/acceptance/application-error-test.js

import { test } from 'qunit';
import Ember from 'ember';
import moduleForAcceptance from 'application-error-test/tests/helpers/module-for-acceptance';
import startApp from 'application-error-test/tests/helpers/start-app';
import sinon from 'sinon';

Ember.Test.QUnitAdapter.reopen({
  /*
    This is a hack, throwing an error in a route must use the error.name
    `TransitionAborted`; otherwise, `Test.adapter.exception(error)` inside
    `onerrorDefault` fails tests via the same error that results in rendering
    the application-error template.
  */
  exception: function(error) {
    var makeItPass = false;
    debugger; // FIXME… CHANGE ^ to true to work around bug
    if (makeItPass /* we mocked a 502 so expected an error, it renders the application-error template… */) {
      console.warn('Expected this error, ignoring (no reason to fail a test)…', error);
    } else /* normal behavior… if (error.name !== 'TransitionAborted') */ {
      return this._super.call(this, error);
    }
  }
});

moduleForAcceptance('Acceptance | application-error', {
  beforeEach: function setup() {
    this.sandbox = sinon.sandbox.create();
    setupFakeServer.call(this);
    mockFetchSession.call(this);
    this.application = startApp();
  },
  afterEach: function teardown() {
    Ember.run(this.application, 'destroy');
    this.sandbox.restore();
    delete this.sandbox;
  }
});

test('with a server error, the application-error template renders', function(assert) {
  let done = assert.async();
  visit('/');
  andThen(function() {
    assert.equal(currentURL(), '/', 'URL is "/"');
    assert.equal(find('.error-message').length, 1, 'Application error template rendered');
    done();
  });
});

function setupFakeServer() {
  this.sandbox.stub(Ember.$, 'ajaxPrefilter');
  this.server = this.sandbox.useFakeServer();
  this.server.autoRespond = true;
}

function mockFetchSession() {
  mockBadGatewayResponse.call(this);
}

function mockBadGatewayResponse() {
  this.server.respondWith('GET', 'api/session', [
    502,
    { 'Content-Type':'text/html; charset=utf-8' },
    `<html>
<head><title>502 Bad Gateway</title></head>
<body><h1 class="error-message-content">Bad Gateway</h1></body>
</html>`
  ]);
}

Pseudo application code

templates/application-error.hbs

<h1>Oops, there was an error…</h1>
<p class="error-message">{{model.message}}</p>

routes/application.js

import Ember from 'ember';

export default Ember.Route.extend({
  model() {
    return Ember.$.ajax({url: 'api/session', method: 'GET'});
  }
});

Be sure to see the README of the example repo. This is the commit that shows the setup for the example app.

pixelhandler pushed a commit to pixelhandler/application-error-test that referenced this issue Jan 9, 2016

pixelhandler pushed a commit to pixelhandler/application-error-test that referenced this issue Jan 9, 2016

@pixelhandler

This comment has been minimized.

Copy link
Contributor

pixelhandler commented Jan 11, 2016

@rwjblue, @stefanpenner does this look related: #11903 and #11469 seems like this issue has the same root cause "RSVP Error failing the test"

@pixelhandler pixelhandler changed the title Test.adapter.exception forces acceptance test for application-error substate to fail Test.adapter.exception forces acceptance test to fail (RSVP error fails test) Jan 11, 2016

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Jan 11, 2016

has the same root cause "RSVP Error failing the test"

This sounds like a symptom, not a root cause.

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Jan 11, 2016

So it appears their is a legitimate failure, but it is confusing that their is no-obvious way to handle it and retain the error page.

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Jan 11, 2016

some relevant docs https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/route.js#L632-L685

But it does not appear like an obvious solution.

@machty how does one gracefully handle such an exception?

@machty

This comment has been minimized.

Copy link
Member

machty commented Jan 11, 2016

Presently if you don't handle the error with an error "action" then if/after it finds an error substate, it continues throwing the error so that it hits the console and error reporters (e.g. Bugsnag) can detect and report. I think people depend on this behavior, so I don't think we can change it.

In general we need to solve the problem of substate testability (a lot of the issues stem from the default behavior of waiting for the entire route transition promise chain to complete), but in the meantime you might consider the following workaround on your application route:

  actions: {
    error(e) {
      this.intermediateTransitionTo('application_error', e)
    }
  }

This basically tells Ember the error has been "handled" while preserving the default behavior. The big downside is that if you have a nested error substate template defined, it'll be ignored, since this error hook will run first before Ember tries to find the most specific error substate route. Hmm... I'm not really sure the best way to go with this one.

@pixelhandler

This comment has been minimized.

Copy link
Contributor

pixelhandler commented Jan 15, 2016

@machty @stefanpenner what about an ember-testing method that will allow you to set the test "exception" handler - Test.adapter.exception. I see this api doc http://emberjs.com/api/classes/Ember.Test.Adapter.html#method_exception but I don't know how to override that, other than using reopen which isn't the way to go for one acceptance test.

Perhaps Ember.Test.registerExceptionHandler and Ember.Test.unregisterExceptionHandler would be needed to…

solve the problem of substate testability

Also, here is some documentation on how I'm using error substates.

@dopin

This comment has been minimized.

Copy link

dopin commented Jan 22, 2016

I've been stuck on this while upgrading Ember. I could solve it with your workaround code. Thank you @pixelhandler !

@ppcano

This comment has been minimized.

Copy link
Contributor

ppcano commented Jan 26, 2016

Currently a solution for this issue is just to overwrite Ember.Test.Adapter.exception which provides a public API for this purpose.
An example is described at rwjblue/ember-qunit#163

I think, the problem comes up with the use of the default Ember.Test.Adapter which throw an exception on a promise rejection and it will make the test to fail.

IMO, this should not be the default behaviour for acceptance tests.

Some possible improvements could be:

  • describe this situation in the documentation & guides for acceptance tests
  • remove this default behaviour in the Test adapter

cc / @rwjblue

@pixelhandler

This comment has been minimized.

Copy link
Contributor

pixelhandler commented Jan 29, 2016

@ppcano Yeah this example seems usable:

let adapterException;
beforeEach() {
  this.application = startApp();
  adapterException = Ember.Test.adapter.exception;
  Ember.Test.adapter.exception = () => null;
},

afterEach() {
  Ember.Test.adapter.exception = adapterException;
  Ember.run(this.application, 'destroy');
}
@backspace

This comment has been minimized.

Copy link

backspace commented Mar 1, 2016

I adapted this code and put it in start-app.js to apply it across all acceptance tests. This has also been causing problems for me in updating, in this case from 1.13 to 2.4. Some of our acceptance tests expect errors from the mock API to test error-handling.

I’m only just encountering this fix now, but I hope I can come up with a way to mark an exception as expected for particular tests to still have the fail-on-error for unexpected exceptions.

@backspace

This comment has been minimized.

Copy link

backspace commented Mar 4, 2016

I found today that this override caused a false positive in my acceptance tests. I use ember-cli-page-object and it usually causes a test to fail when it can’t find an expected element to make an assertion on. Instead, this override was ignoring the exception that would cause the test to fail, so I inadvertently broke things with a refactoring that I only caught in manual testing.

It turns out I only had one test that needed this override, one that tests handling of a 500 from the API. So I’m able to narrowly apply the override to just that test.

Cautionary tale!

@john-kurkowski

This comment has been minimized.

Copy link

john-kurkowski commented Mar 9, 2016

swatijadhav added a commit to crossroads/app.goodcity that referenced this issue Mar 14, 2016

swatijadhav added a commit to crossroads/app.goodcity that referenced this issue Mar 16, 2016

swatijadhav added a commit to crossroads/app.goodcity that referenced this issue Mar 17, 2016

@pixelhandler

This comment has been minimized.

Copy link
Contributor

pixelhandler commented May 6, 2016

@john-kurkowski yeah thx, so there are more than one way to work-around the test bug.

@rwjblue Do you think it's worth calling this a BUG ? or Do we just treat this as something that needs Documentation for work-arounds?

Some possible improvements could be:
describe this situation in the documentation & guides for acceptance tests
remove this default behaviour in the Test adapter

@locks what do you think about documenting work-arounds, seems odd to to that but if we don't call this a bug then that is the only resolution.

@aquamme

This comment has been minimized.

Copy link

aquamme commented May 11, 2016

In order to avoid the problem that @backspace encountered, I modified the code @pixelhandler provided:

let adapterException;
beforeEach() {
  this.application = startApp();
  adapterException = Ember.Test.adapter.exception;
  Ember.Test.adapter.exception = arg => {
    if (arg !== 'expectedException') {
      throw arg;
    }
  };
},

afterEach() {
  Ember.Test.adapter.exception = adapterException;
  Ember.run(this.application, 'destroy');
}
@pablobm

This comment has been minimized.

Copy link
Contributor

pablobm commented Jun 19, 2016

Thank you @pixelhandler and @aquamme. I have taken your code, generalised it into a module and published it as https://gist.github.com/pablobm/5ec0fee8a877badaca2c11bf4751e857

@pixelhandler

This comment has been minimized.

Copy link
Contributor

pixelhandler commented Jul 22, 2016

I'm going to close this in favor of just using the work around.

@Leooo

This comment has been minimized.

Copy link
Contributor

Leooo commented Sep 6, 2016

It may help someone: in my case I had to use

let adapterException;
beforeEach() {
  this.application = startApp();
  adapterException = Ember.Test.adapter.exception;
  Ember.Test.adapter.exception = () => {
    return null;
  };
},

afterEach() {
  Ember.Test.adapter.exception = adapterException;
  Ember.run(this.application, 'destroy');
}

instead of

let adapterException;
beforeEach() {
  this.application = startApp();
  adapterException = Ember.Test.adapter.exception;
  Ember.Test.adapter.exception = () => null;
},

afterEach() {
  Ember.Test.adapter.exception = adapterException;
  Ember.run(this.application, 'destroy');
}

t

@fiftoine

This comment has been minimized.

Copy link

fiftoine commented Feb 24, 2017

overriding Ember.Test.adapter.exception is fine but make tests pass even if an exception is thrown.

With an helper like

Ember.Test.registerAsyncHelper('throwsAdapterError', function(app, assert, closure) {
  let adapterException = Ember.Test.adapter.exception;
  var exceptionThrown = false;
  Ember.Test.adapter.exception = function(){
    exceptionThrown = true;
  };
  closure.apply();

  return andThen(function(){
    assert.equal(true, exceptionThrown);
    Ember.Test.adapter.exception = adapterException;
  });
});

You can target user interactions that will throw exceptions with

throwsAdapterError(assert, function(){
        click('...');
 });

There is surely a better way to implement this but this works fine for me.

@workmanw

This comment has been minimized.

Copy link
Contributor

workmanw commented Feb 24, 2017

@fiftoine I actually filed another issue (#14864) and opened a PR to fix this (#14898). It's still waiting on a code review.

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