Closed lolmaus closed 7 months ago
Note that this is not a problem for ember-mocha
. Mocha-driven tests do not crash when Ember Data rejects a promise.
Here's what I figured out:
assert.throws
, but:
Ember.onerror
is an old and quite poorly documented feature of Ember:
ember-mocha
does not seem to use it.ember-qunit
does use it (see below), but does not document it, keeping us clueless.Ember.onerror
is a candidate for deprecation and should be avoided. @amk221 kindly helped me understand that it has special behavior in ember-qunit
: catching an error in Ember.onerror
would prevent QUnit from failing a test!Ember.onerror
is not catching all errors. In case it does, you should see a failing test case. To avoid this, you need to re-throw errors after catching them in Ember.onerror
.@amk221 pointed out that Ember.onerror
only catches errors that are thrown within the Ember runloop. For some reason, this code:
async someMethod() {
try {
await this.store.query(/*...*/);
} catch (e) {
throw e;
}
}
...would throw the error outside of the runloop, and Ember.onerror
will ignore it!
The solution is to wrap it with run
from '@ember/runloop'
. This is the first time I've had to use run
in app code.
Now I have control over errors in tests again, which I lost after Ember.Test.adapter.exception
was removed.
Big thanks to @amk221 for taking time and effort to help me figure this out.
Though my immediate problem is resolved, I'm gonna leave this issue open because:
Ember.onerror
behavior in tests needs to be documented.Ember.onerror
. We probably need an RFC to introduce a tool that can be imported and allows pub/sub, so that it can be used without overwriting other uses.Just to follow up on the conversation @lolmaus and I had on discord
The pseudo code that helped was:
Ember.onerror = error => {
if (/* i want to ignore the error */) {
return; // 1
}
if (Ember.testing) {
throw error; // 2
}
}
FWIW, setupOnerror
(from @ember/test-helpers
) is specifically attempting to make that one off setup easier.
To specifically answer the original question:
How do I prevent QUnit from crashing on unhandled errors? What is the modern replacement for overriding Ember.Test.adapter.exception?
You would do something akin to:
import { setupOnerror } from '@ember/test-helpers';
test('....', async function(assert) {
setupOnerror((error) => {
// do nothing, throw, whatever you want
});
await visit('/whatever');
// ...more stuff...
});
@rwjblue Thanks, that works great, but only if you use RSVP. Here is a basic example demonstrating the problem.
Imagine you have an app that used to have:
actions: {
doSomethingAsync() {
return this.service.doSomething().then(() => {
this.flash.add('done');
}).catch(error => {
this.flash.add('failed');
throw error;
});
}
}
...but now you want to modernise to use async/await...
@action
async doSomethingAsync() {
try {
await this.service.doSomething();
this.flash.add('done');
} catch(error) {
this.flash.add('failed');
throw error;
}
}
setupOnerror
is never called, and the opportunity to squelch the thrown error never happens, so the test fails.
I think it fully depends on the actual test in question. Generally speaking, there is no magic bullet for handling native promise rejections. The fact that it is possible in RSVP has been a convenient crutch for us for a while, but just doesn't match our future reality.
there is no magic bullet for handling native promise rejections
Please let me reiterate as I'm struggling to understand. Point out where I make a mistake or a false assumption.
The problem is that QUnit chooses to crash tests on all unhandled promise rejections (with exceptions mentioned above).
Given that QUnit is part of Ember apps by default (and the only real alternative, ember-mocha
, is falling behind and not quite usable), we need a solution for this.
If the case of failed tests on unhandled native promises "works as intended", then we need guidelines how to deal with unhadled promises. E. g. should the developer always take care of catching all promises that they spawn?
It's not that obvious, because some async methods are hooks which are handled by Ember internals (sometimes too aggressively — you might not see the error at all, or it might be caught and rethrown elsewhere with an entirely irrelevant stack trace), but others like an action in @amk221's example are not handled automatically.
So two points here:
throw error
with something like this.metrics.trackError(error)
. Is that what we're supposed to do?PS Thank you for looking into this matter.
@lolmaus - I'll try to reply to the various sections independently (as there are a few separate points you are making).
The problem is that QUnit chooses to crash tests on all unhandled promise rejections (with exceptions mentioned above).
FWIW, I think this is the correct stance for QUnit to take (and IIRC I implemented it waaaaaaay back in 2017 https://github.com/qunitjs/qunit/pull/1241). Unhandled exceptions (and unhandled promise rejections) are bad, and in most circumstances point to an actual problem in the application.
If the case of failed tests on unhandled native promises "works as intended", then we need guidelines how to deal with unhandled promises
I totally agree with you here! We absolutely should document how to handle situations like @amk221's.
It's not that obvious, because some async methods are hooks which are handled by Ember internals (sometimes too aggressively — you might not see the error at all, or it might be caught and rethrown elsewhere with an entirely irrelevant stack trace), but others like an action in @amk221's example are not handled automatically.
I agree that it is unclear how Ember's internals handle unhandled rejections. I think we should update the documentation for all async route hooks to explicitly say what happens when an exception occurs. For example, if an exception occurs during model
and you have an error
substate or error
action that rejection is automatically handled for you (this is why you don't commonly see the problem you are reporting for that category of code). I also think that doing that documentation will significantly help folks think about how exceptions / rejections are handled by the system (and therefore how they should be handled by their own code).
A typical Ember developer does not have a habit of catching promises because Ember guides never state that they should, and it works in most cases until it suddenly doesn't, and it's absolutely frustrating.
To reiterate, I agree with you. We should increase both API and guide documentation for exceptions/rejections and how application developers should handle those situations. FWIW, I do not think the answer is "never leave them uncaught" (as sometimes you really do intend for an error / rejection to bubble all the way up loudly).
How do we even handle promises? Take @amk221's example. What can be done to resolve the problem? The only thing I can think of is replacing throw error with something like this.metrics.trackError(error). Is that what we're supposed to do?
I am totally on board with the idea that we should identify specific use case / scenarios, and solve for them. That will help us know what to document (and where). Unfortunately, the example doesn't have enough information. What is triggering this action? What does the test look like? What kind of test? etc.
Ultimately, I think we should (mostly in order):
Oh, also, I wanted to point out that this bit (from the original issue text) just isn't a factor here:
Previously, it was possible to override Ember.Test.adapter.exception and prevent tests from failing on uncaught assertions. The best part is that you could check for a condition (e. g. a certain error code or message) and re-throw an error if it was not the one you were expecting.
Anything that Ember.Test.adapter.exception
would have caught is handleable (IMHO in a better way) with setupOnerror
from @ember/test-helpers
.
The conversation here recently is specifically about native promises (Ember.Test.adapter.exception
could only ever catch RSVP
promises and sync errors thrown from the run loop).
Thanks!
the example doesn't have enough information
I'll try and explain a bit more... I use the errors for flow control 🙈, e.g.
@action
attempt() {
return this.validate()
.then(() => this.save())
.then(() => this.modal.close(();
}
this.validate
throws, so that this.save
never happens. It is that that is causing me grief. I realise it's frowned upon to use errors for flow control, so I could stop doing that. But I thought I'd let you know that's where my pain point was.
@amk221
I thought I'd let you know that's where my pain point was.
Gotcha. Where are you calling this.attempt()
from? What kind of test is this?
Would you mind making a repo with a small demo?
I've also come across this issue today, where setupOnerror
couldn't catch the exception. It slipped through QUnit and caused the test to fail. Good thing is I'm quite clear on what I did and have a temporary workaround.
The "expected behavior": I do abort a transition under a specific condition transition.abort()
.
Original Tests:
const lock = confirmService.lockCurrentRoute();
await visit('/route-b');
assert.equal(currentURL(), '/route-a');
(I'm trying to visit route-b
which is blocked). This resulted in:
Which then I wanted to test with:
setupOnerror(function (error) {
assert.ok(error); // or even more precise: assert.equal(error.name, 'TransitionAborted');
});
const lock = confirmService.lockCurrentRoute();
await visit('/route-b');
assert.equal(currentURL(), '/route-a');
setupOnerror();
Here is the actual workaround:
const lock = confirmService.lockCurrentRoute();
try {
await visit('/route-b');
} catch (error) {
console.log(error);
}
assert.equal(currentURL(), '/route-a');
With the log showing this:
Hope this brings some insights.
Would you mind making a repo with a small demo?
Just encountered the same issue while upgrading ember-qunit from 4.6.0 to 5.1.3
Same here, is this planned to be fixed/updated by any chance?
I run into another case of errors, which aren't going through Ember.onerror
and therefore can not be handled with setupOnerror
in tests: Event listeners assigned with {{on}}
modifier.
Assume we have this template-only component:
<button {{on "click" @onClick}} type="button">
click me
</button>
Now we want to write a test for it, that it does not silently catch errors thrown by function passed to @onClick
argument. My first attempt would look like this:
test('it does not catch errors thrown by @onClick handler', async function (assert) {
const expectedError = new Error();
setupOnerror((error) => {
assert.step('error is thrown');
assert.equal(error, expectedError);
});
this.set('clickHandler', () => {
throw expectedError;
});
await render(hbs`<Button @onClick={{this.clickHandler}} />`);
await click('button');
assert.verifySteps(['error is thrown']);
});
But the error is not handled by Ember.onerror
. Therefore setupOnerror
is not working. Without monkey-patching QUnit's internals it is impossible to test this.
Please find a reproduction here: https://github.com/jelhan/reproduce-component-event-handler-throws-testing-issues/compare/b95001721b9b027c444926db168975ed846fdac3...master It also contains a very hacky work-a-round, which requires monkey-patching QUnit.
This is driven by a real-world use case in Ember Bootstrap. A component provided by Ember Bootstrap silently catches errors of the event handler. This is considered a bug. Fixing the bug is easy. Adding tests for it is way more challenging. And even worse: Other existing tests, which tests behavior in case event handler rejects, are affected by that bug fix as well. So fixing the bug without adding tests is not an option. It would require even to disable existing tests. :sob:
Hi @rwjblue ! Any progress on this one?
Ran into this problem again with a test that intentionally returns HTTP 500 from a mocked API endpoint. QUnit chooses to crash a test on a rejected promise.
setupOnerror
is not doing anything for this case.
An HTTP 500 error should be not an unexpected case, and this is how it is handled gracefully (simplified):
{{#if this.submitTask.isRunning}}
Submitting...
{{else if this.subitTask.last.isError}}
Something wrong happened:
{{format-error this.submitTask.last.error}}
{{/if}}
I was able to resolve it like this.
Before:
<button {{on "click" (perform this.submitTask)}}>Submit<button>
After:
<button {{on "click" this.submit}}>Submit<button>
@action
async submit(): Promise<void> {
try {
await this.submitTask.perform();
} catch(e) {
// We don't want tests to unconditionally crash when this happens.
// The error will be handled by `submitTask.last.isError`
}
}
So this action only exists to wrap the Ember Concurrency task with a do-nothing try-catch block.
Apparently, the {{perform}}
helper of Ember Concurrency should be considered faulty because it won't catch failed or cancelled tasks, causing tests to fail. 🤷♂️
I don't think that rejected promises should crash tests. Handling it requries too much runtime boilerplate that is absolutely unnecessary for production and only exists to please QUnit.
As for aborted transitions, I don't even know how to wrap them with a try-catch.
CC @simonihmig
This is what I did for a very specific use case of purposefully rejecting promises in the library.
@snewcomer I came here to say https://api.qunitjs.com/extension/QUnit.onUncaughtException/, which is available and public API as of qunit@2.17.0, works nicely.
FWIW, I've opened https://github.com/DefinitelyTyped/DefinitelyTyped/pull/57770 to update the qunit types to include this hook.
I've started a PR to finally fix this: ember-test-helpers#1194. I would appreciate your thoughts!
I am getting issue with if(Ember.testing ) { throw error} condition - I have added the Ember.onerror in my App.js route, also there is one component in node-module where there is Ember.onerror is use with if(Ember.testing ) { throw error}. So when I generate the error it goes to inside the node-module component, whereas i expected it will trigger my App.js route.
Please can someone help me how to keep my App.js onerror function to trigger instead of the node-module component?
There are some techniques / platform APIs to catch errors: https://github.com/emberjs/ember-test-helpers/issues/1128#issuecomment-1920196763
I don't know the timing of QUnit.onUncaughtException
and the browser-based unhandledrejection
event (what the above explores), but given that both these tools now exist, I think we can close this issue (as the problem isn't really anything wrong with ember-qunit, per se (feel free to disagree tho! :sweat_smile: I can be convinced!)).
There is a desire among a good few (many?) to have setupOnerror
also listen to this type of error event, so maybe there is a PR in someone's future to add that capability? 🥳
It seems to me like setupOnerror
is a poor mans try-catch. Why can't we do this:
test('something we know will fail', async function() {
assert.expect(1);
try {
await visit('/');
} catch(error) {
// squelch, since we are testing an error scenario
}
});
What we should target for:
await assert.rejects(
visit('/')
);
await assert.rejects(
click('button')
);
But I fear there are architectural limitations in Ember.js itself, which prevents it for most use cases...
Last time I tried working around the limitations with QUnit.onUncaughtException
and unhandlerejection
browser event, I run into many issue. And it was breaking over time with upgrades of Ember, QUnit, and other libraries. Maybe it got better. But it's still far away from a good testing story for error cases.
Why can't we do this
This is actually how you test beforeModel transitions aborts :sweat_smile: See: https://github.com/emberjs/ember-test-helpers/issues/332
which prevents it for most use cases...
yeah, we're hoping all the router-related limitations here go away when we get to Polaris
Hi!
In our app, we catch errors in order to display flash messages to the user, letting them be aware of an error and offering a chance to retry.
We choose to re-throw the error after displaying the flash message. This is because we want the error to be caught by an error tracking service such as Sentry, Bugsnag, TrackJS, Raygun, etc.
The problem is that QUnit tests would crash on any uncaught error, making it impossible to test error reporting.
There are two userland workarounds from this issue:
Previously, it was possible to override
Ember.Test.adapter.exception
and prevent tests from failing on uncaught assertions. The best part is that you could check for a condition (e. g. a certain error code or message) and re-throw an error if it was not the one you were expecting.But according to ember-qunit source, it is no longer possible in modern Ember:
How do I prevent QUnit from crashing on unhandled errors? What is the modern replacement for overriding
Ember.Test.adapter.exception
?