Closed agilgur5 closed 5 years ago
browser-env
works for supporting ava
(per https://github.com/avajs/ava/blob/master/docs/recipes/browser-testing.md), but jest
actually has built-in support for jsdom
turns out: https://jestjs.io/docs/en/configuration#testenvironment-string. Per the note at the bottom however, it's locked to JSDOM 11 for now, but could install jest-environment-jsdom-fifteen
for latest support if needed.First big problem I ran into when creating the test harness was babel
support. While babel
support is built into jest
with babel-jest
, it's currently defaulted to Babel 7 and we're still on Babel 6. I don't quite want to upgrade the build toolchain yet (tests would be useful to ensure the output stays the same and that's a big project on it's own, don't want to mix that with tests), so I used the workaround available in https://jestjs.io/docs/en/getting-started#using-babel to install babel-jest@23
to work with Babel 6.
...But then I ran into a bunch of issues with configuration:
babel-jest@23
won't actually be used just by installing it, there's a bug that requires configuring it a bit.babel-jest@23
also can't read .babelrc.js
, only 24 can, so I had to use .babelrc
JSON format for configuration.
This was extremely confusing why it wasn't transpiling correctly after I finally got babel-jest@23
to work and I actually found out by trial-and-error (trying babel.config.js
and .babelrc.js
in various locations and eventually .babelrc
) before I found that issue.babel-loader@6
doesn't support .babelrc
files, so I couldn't centralize all babel config in one place and had to duplicate the config in Webpack (that's already duplicated in prod v. dev configs). Couldn't upgrade to babel-loader@7
as while it supports Babel 6, it doesn't support Webpack v1... so many compatibility issues... 😅 😅 P.S. It turns out upgrading to Babel 7 in this specific repo shouldn't be too difficult as I think the only proposal we're using is class-properties
. Per the upgrade guide for stage-2
would just add "plugins": [ ["@babel/plugin-proposal-class-properties", { "loose": true }] ]
and remove stage-2
from "presets"
. Should replace react
with @babel/preset-react
too.
babel-preset-es2015
isn't quite as clear and took a bit of research to grok, but per the @babel/preset-env
targets
config docs, should just be able to use the default ({}
) to transpile all ES2015+ code, which should be an equivalent transform to preset-es2015
. If any polyfills are used in this library (I don't think so), "useBuiltIns": "usage"
would be optimal so that they're re-used and not duplicated amongst users other polyfills.
Oh yea, so I decided to use jest
instead of ava
because I figured it would be easier to configure due to all the built-in-ness of it. I also found a way to import jest in tests so it wouldn't rely as heavily on globals (but this required configuration and still pollutes global scope as the PR or similar has yet to be accepted, so still not quite as ideal as ava
).
Now, of course, looking at all the above stuff, there still ended up being quite a bit to configure for the specific environment here, so that kinda sucked 😕 .
The main differences seem to be that I don't need to install/configure something like nyc
for code coverage as it's built into jest
(and maybe will have less issues than nyc
? 🤞) and don't need to configure jsdom
either. Also, ava
seems to only be able to be configured via package.json
and doesn't support configuration through a separate file (which I strongly prefer), whereas jest
has jest.config.js
.
jest
of course also has various expect
matchers out-of-the-box (though I currently don't like the flexibility this feature entails) and also supports mocking / spying out-of-the-box too (which, while should be minimized, is indeed useful).
And of course, just as I got the test harness to finally work, it bugged out on canvas
support with the infamous TypeError: Cannot set property 'fillStyle' of null
.
I initially thought it would be automatically fixed if I upgraded to jest-environment-jsdom-fifteen
, but that actually didn't solve anything. jsdom
v13+ only supports canvas
v2+, so I tried installing canvas-prebuilt@2
but that didn't work. Not sure if that's because prebuilt@2
is still in alpha (didn't try regular canvas@2
) or if there might be a bug in jest-environment-jsdom-fifteen
.
Anyway, I downgraded back to the included version of jsdom
(v11) in jest
and then just installed canvas-prebuilt@1
and it was fixed and my simple mount test finally worked, phew!
We'll see if I might need some later versions of canvas
to support all the features, but for now it works, so I'll commit and push a tests
branch before going to bed. This took a lot more time than I thought (and I thought it would take a decent amount of time!), so I guess I won't be getting to actual adding any real testing today 😕😞.
Oh also worth to note is that jest
is running mad slow right now, 6+ seconds for a single test that just mounts the component 😮 ... Not sure if this is because of jsdom
or canvas
or because of old versions of both or babel-jest
or what, but it almost makes me want to try setting up ava
as well just to see if it runs faster 😕
It seems like jest
is spending most of its time in init phases and not in actually running the tests, but I'm not sure what that means.
So canvas-prebuilt@2
actually isn't supported by jsdom
v13+ and apparently canvas@2
includes a pre-built version, so canvas-prebuilt
is only needed when @1
compatibility is needed.
I was getting some errors with dataURL fixtures when I realized the above^. But seems like those error isn't because of the canvas
version as far as I can tell. toDataURL
in the browser and toDataURL
in node-canvas tests return two different things after the same fromData
call, so I guess the implementations are just slightly different? I didn't expect dataURLs to be super accurate anyway, which is why I started with pointGroups
/fromData
first, but didn't think they'd be different either 😖
I also had some trouble with trimming the canvas, getting IndexSizeError: The source width is 0
, but that turned out to be because I didn't give the SignatureCanvas element a width or height (edit: forgot I don't have any wrapper CSS in tests for percentage size). The lack of width and height was also why I was getting a dataURL of data:image/png;base64,
with nothing else -- once I changed that the dataURLs actually existed, but still weren't the same.
wrapper methods and get
methods are all tested now with 80%+ code coverage. Just have left:
componentDidUpdate
clearOnResize
on
& off
Got to 100% line/statement coverage last night, some notes:
Had a decent time-sink figuring out how to check prop and options equivalence in SignatureCanvas and SIgnaturePad. First wrote an un-reusable for loop to go through and expect
each prop before I found toMatchObject
. Then used toMatchObject
a bit wrong (wrong order, thought I had to spread SigPad so it's of Object class instead of SigPad class too, woops) before figuring out how to correctly use it.
toDataURL
stuff I couldn't figure out in the above comment because no regenerator runtime and wrapping with and resolving promises plus assertion count is annoying, but then found something different while using Enzyme's setProps
-- which is the done
callback arg of test
. This appears in the Async "Intro" of Jest docs but not in the "Async Example" in the "Guide", so I just never saw it until then.Turns out window.resizeTo
is not implemented in jsdom
, so I had to polyfill it using some code similar to this. There also doesn't appear to be an NPM package for this?
Turns out you can't check what listeners are attached to the window (i.e. for on
/off
tracking) so I had to do some mocking instead.
Also changed some code style things:
<fixtureName>F
instead of separate variables. It got a little unwieldy and confusing even with just a handful. The <prefix>F
naming helps with code readability and grouping makes things more understandable and keeps fixtures used together and not re-used along the wrong boundaries.describe
and it
as it fit well enough for resizing and on/off, but doesn't necessarily fit the props and wrapper methods tests (maybe I need another describe
? or just need to re-word? maybe need to shift my thinking a bit as this happens with some frequency with me with test naming, though idk how common it is).Found another weird issue that I figured out which was whether Enzyme's setProps
is async or sync. The docs have a callback arg that make it seem async (like React's setState
), but I accidentally wrote it both as async and sync in the code in different places... and it worked in both 😮 😖 .
The callback was indeed added because it was (sometimes?) working async per this issue. But the currently last comment on that issue says that it was updated to work synchronously in v3.4.3 for ease of testing. But v3.4.3 changelog doesn't explicitly say that...
Real confusing, but I just rewrote my code to use it sync everywhere then as it's more readable and works fine that way 🤷♂
Oh and forgot to mention that I looked into Code Coverage reporting providers and set-up CodeCov as it seemed to very popular and easy (and no need for tokens for public repos w/ public CI) with a one-line bash uploader.
Can view the reporting here: https://codecov.io/gh/agilgur5/react-signature-canvas/ There's also a badge on the tests branch (though it is set to only check master, so is reporting as "unknown" right now as it hasn't landed in master)
Place for notes as I'm implementing tests on the
tests
branch.Some initial notes:
enzyme
for component testing as we actually do need the ref in this library's case to test instance methods, so can't usereact-testing-library
. We also do need to test actually mounting to the DOM (due tocanvas
usage), soreact-test-renderer
doesn't really suffice either.canvas
like thecanvas
package orcanvas-prebuilt
, but apparently need fulljsdom
support for enzyme (https://airbnb.io/enzyme/docs/guides/jsdom.html). The set-up forjsdom
forenzyme
however seems equivalent to whatbrowser-env
does, so that can be a replacement.