-
Notifications
You must be signed in to change notification settings - Fork 398
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
Upgrade to Jest 21.0.2 #3088
Upgrade to Jest 21.0.2 #3088
Conversation
Codecov Report
@@ Coverage Diff @@
## master mozilla/addons-frontend#3088 +/- ##
=======================================
Coverage 95.17% 95.17%
=======================================
Files 156 156
Lines 2839 2839
Branches 552 552
=======================================
Hits 2702 2702
Misses 118 118
Partials 19 19 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
tests/unit/core/server/testServer.js
Outdated
// unhandled rejection warning that Jest 21.0+ converts into an error. We | ||
// do want to throw exceptions on the server. See also: | ||
// http://thecodebarbarian.com/unhandled-promise-rejections-in-node.js.html | ||
error.catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty naughty, like something that should be labelled with HACK:
haha.
But the reasoning seems fine to me. I honestly forget why we want to throw on the server though, maybe you could add a comment explaining why?
tests/unit/core/server/testServer.js
Outdated
@@ -180,6 +195,12 @@ describe(__filename, () => { | |||
.end(); | |||
|
|||
expect(response.statusCode).toEqual(500); | |||
mockUserApi.verify(); | |||
|
|||
// This ensures the error caught is the expected one, that is the one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"caught is the expected one, that is the one" should be simplified to "caught is the expected one, from"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumar303 what do you think of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring this out. I'm sure the new Jest will have some nice features but if this patch slows you down then we can always revisit it later.
The fact that we have an unhandled promise rejection on the server does raise a bit of a flag for me. As far as I can tell, that's buried in Express but I'm not sure. Since we do have an error handler in the server code I can't think of a reason why we should be leaking an unhandled promise rejection. Perhaps I'm not fully understanding the problem though.
tests/unit/core/server/testServer.js
Outdated
}); | ||
|
||
it('returns a 500 error page when retrieving the user profile fails', async () => { | ||
const errorMessage = 'example of an API error'; | ||
const error = Promise.reject(new Error(errorMessage)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this is a confusing workaround.
I think it would be helpful to make a test helper for this that looks like:
const errorResult = rejectedPromise(new Error(errorMessage));
This would allow us to put the comments in the helper function itself.
We use Promise.reject()
all over the test suite so I don't understand why only this test needed updating. Is it something to do with how this promise runs in a separate process? I.E. the test server promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A helper works for me.
I think the problem comes from the server-side rendering indeed. We boot a real Express app and I think node or jest loses track of the promise and thinks it is not handled (that is what I understand from this blog post: http://thecodebarbarian.com/unhandled-promise-rejections-in-node.js.html, especially the section about to sinon and stubs).
I have attached .catch()
in many different places and tried a few other try
/catch
combinations. I am not able to catch this "unhandled rejection warning".
Not sure if we can do anything else, because it seems we have to hook into process.on('unhandledRejection', () => {})
. That is also what Jest does since version 21 actually. If I comment the Jest code related to that, no more issue... I tried to catch the error in the sagas, but that was not working either.
I don't think this is a perfect solution though. One more thing, I used node 8 too and could not generate unhandled promise warnings using the app for real, but the test suite was failing like on Travis.
tests/unit/core/server/testServer.js
Outdated
// This ensures the error caught is the expected one, that is the one | ||
// from `userProfile` API call. | ||
const { loadState } = store.getState().reduxAsyncConnect; | ||
expect(loadState.ServerBase.error.sagaStack).toContain(errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this assertion necessary? I would rather we didn't rely on reduxAsyncConnect
because it will be removed in https://github.com/mozilla/addons-frontend/issues/2356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not come with something better to be 100% sure the error caught came from the promise rejection, not something else (example: a check in the reducer called right after callApi()
saying the profile
argument was not provided).
I did that because I found a few fixes to make the test suite all green but that was not correct. For instance, it would return a 500
due to an error related to the reducer rather than the saga.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a high-level integration test so I don't think it's crucial that we make an assertion about the exact error that triggered the 500. In other words, we won't rely on this specific test to tell us that fetching a user profile fully works as expected. We have better unit tests for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, I can get rid of the promise "hack" I guess by not rejecting the promise but resolving it with empty/null data. The reducer will throw an error and this error will be caught, correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we will need to set up a rejected promise in a server test sooner or later so setting a precedent for how to do it would be nice. I prefer tests that set up explicit errors rather than trying to create the error by side effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly the problem (and remember what I have done last week), it is not because of the use of Promise.reject()
. This, I am pretty sure, works and is handled somewhere in the code. I tend to think this side-effect is a false-positive and would be hard to reproduce in the app.
tests/unit/core/server/testServer.js
Outdated
|
||
expect(response.statusCode).toEqual(200); | ||
expect(api.token).toEqual(token); | ||
expect(user.id).toEqual(42); | ||
expect(user.username).toEqual('babar'); | ||
mockUserApi.verify(); | ||
|
||
const { loadState } = reduxAsyncConnectState; | ||
expect(loadState).toEqual({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a safety measure since you're relying on it in the next test? I'd rather not rely on this state because it will be removed in https://github.com/mozilla/addons-frontend/issues/2356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed here but the check is quite useful to be sure the rendering was successful.
Unless there's a really strong cause for concern, I would suggest that we go ahead with this, the patch is very concise. If there's something that we want to investigate later then we should file an issue to come back to, but otherwise move forward. @kumar303 does that work for you? |
@kumar303 any thoughts ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this up in #3178
Wow, I didn't know this turned into such a simple patch \o/
I guess the uncaught exception was a red herring? I'm not seeing it locally anymore.
@@ -170,7 +170,7 @@ describe(__filename, () => { | |||
mockUserApi | |||
.expects('userProfile') | |||
.once() | |||
.returns(Promise.reject(new Error('example of an API error'))); | |||
.rejects('example of an API error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be rejecting with an error instance, not a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks @kumar303 |
This PR adds Jest 21+ compatibility.
Since Jest 21.0+, Node unhandled rejection warnings are caught and converted into errors (see: jestjs/jest#4016). One of our test cases rejects a
Promise
to test error handling on the server, but Jest now detects that the promise is not handled correctly and fails the test case, also causing subsequent tests to fail.This behavior is not sound asredux-saga
should handle promise rejections, and also ourcallApi
implementation does that too. There are manytry/catch
everywhere so I decided to fix the test case with a mutation of the promise, letting Node think the rejection is handled (see: http://thecodebarbarian.com/unhandled-promise-rejections-in-node.js.html). This does not affect the original behavior, only silent the warning, allowing the test case to pass.