Unit tests: upgrade jsdom to v11 to fix breaking unit tests for Network #3603
Conversation
|
To be clear about it: this fix is required to make |
test/canvas-mock.js
Outdated
| * In particular, this takes care of a 'helpful' message about support of `getContext()`, which in | ||
| * practice is a complete eyesore. It's not a problem in our case, because right here we're using | ||
| * our own canvas mock to deal with it. | ||
| * The unit test run fine with or without this message, it just gets in the way. |
There was a problem hiding this comment.
This comment could be as short as "Supresses getContext warning, ..."
And in the "..." Post the original message
There was a problem hiding this comment.
the original message is an exception trace at least 15 lines long, would prefer not to add that in full. The first line of that is in the code.
There was a problem hiding this comment.
Ok I still think this comment could be shortened considerably
There was a problem hiding this comment.
OK will shorten.
test/canvas-mock.js
Outdated
| // Only 'error' is overridden which for current purposes is fine. The other calls we'll deal | ||
| // with when they crash, in the event that they get used. | ||
| let myConsole = { | ||
| error: (msg) => { |
There was a problem hiding this comment.
Yes, sure. But it's not used here so YAGNI until you do.
There was a problem hiding this comment.
Oh I see. I misunderstood and thought we were suppressing warnings by not including them. But now I understand that until a warning is invoked by a test, it's pointless to add it to the mock.
There was a problem hiding this comment.
mock upon mock upon mock....pretty soon it's going to be mocks all the way down.
|
@mbroad Fixed the comments, have a look again? |
…rk (almende#3603) * First working version * Fix completed for jsdom getContext() issue * Fixed commenting for review
The handling of
getContext()injsdom v9was broken, and caused unit tests forNetworkto fail.The issue has been fixed in
jsdom v11, so I upgraded it.This issue also deals with the fallout of this upgrade:
jsdomneeded to be upgraded as well, notablyjsdom-global.mocha-jsdomwithjsdom-global. The former has not been updated to deal with the latestjsdomversions.before-code to the init method ofcanvas-mock, it was getting too unwieldly..gitignore, not necessarily related to current issueFurther:
jsdom v11issues a 'helpful' message thatgetContext()is not supported withoutcanvas. But we've got the canvas mock in place now, which replaces that. The jsdom message does not break the unit test but is an eyesore (exception output), so effort has been made to bury it.