Fix hapi upgrade #25723
Fix hapi upgrade #25723
Conversation
jasonrhodes
force-pushed
the
fix-hapi-upgrade
branch
from
7045b0a to
346a6fa
Compare
|
|
||
| beforeEach(() => { | ||
| consoleError = window.console.error; | ||
| window.console.error = jest.fn(); |
There was a problem hiding this comment.
Shouldn't this be global instead of window (there is no window object in Node)
There was a problem hiding this comment.
Afaik the mock will be cleaned up automatically if you use jest.spyOn:
jest.spyOn(global.console, 'error')There was a problem hiding this comment.
Hm good point, although it works 🤔 probably because jsdom env running in jest?
There was a problem hiding this comment.
ah, that might be. Can you check if global works too? That seems more "right".
There was a problem hiding this comment.
This should work:
jest.spyOn(global.console, 'error').mockReturnValue(undefined);There was a problem hiding this comment.
I'm not sure if spies can have their return values changed? But even if they can, the damage console.error does is a side effect and not its return value, so this won't help -- the method has to be replaced.
I'll switch it to global though if that still works! 👍
There was a problem hiding this comment.
undefined is the return value of console.log(). It's just a shorthand for this:
jest.spyOn(global.console, 'error').mockImplementation(() => undefined)There was a problem hiding this comment.
Right, but console.error already returns undefined all on its own right? It's the side effect of printing to stdout that I'm trying to suppress, and changing its return value won't do that.
There was a problem hiding this comment.
Both of these will suppress the side effect:
jest.spyOn(global.console, 'error').mockImplementation(() => undefined)
# and
jest.spyOn(global.console, 'error').mockReturnValue(undefined);Simply doing this won't:
jest.spyOn(global.console, 'error')There was a problem hiding this comment.
Ok looked into this and TIL you can stub a function in jest using spyOn ... did not expect mockImplementation that to work like that!
| return; | ||
| } | ||
|
|
||
| throw new Error('Route succeeded when it should have failed'); |
There was a problem hiding this comment.
There is a built-in for this:
expect(() => route[0].handler(mockReq)).toThrowError(BoomError);There are different matchers. You might have to modify it a bit to make it work.
There was a problem hiding this comment.
Yeah, unfortunately the ones I tried didn't work here because I want to test things about the rejected error. Where does BoomError come from in your example?
There was a problem hiding this comment.
Where does BoomError come from in your example?
Fantasy land :) I just expected something like that to be available. But this works:
await expect(route[0].handler(mockReq)).rejects.toMatchObject({ isBoom: true });
💚 Build Succeeded |
|
@sqren just pushed changes to those tests based on your feedback, great ideas! 👏 |
|
Great! LGTM |
💚 Build Succeeded |
* Fixes consistent error handling in routes, adds tests for error handling * Upgrades to failure tests per review
Closes #24844
Summary
This fixes the Hapi problem outlined in the #24844 ticket. I chose to use
throwoverreturnto be explicit about re-throwing the error.I also added tests for all of our routes to make sure we throw that error correctly and don't blow things up in the client by failing to do that. The tests are a bit overkill, but it seemed good to have a failsafe there.