Add Tests & 100% Code Coverage (+ some related standardization) #9
Conversation
- ensure that width/height of canvas are trimmed down after drawing a
purple rectangle in the center
- add test script that uses jest
- (ci): change CI to run test and test:pub
- configure jest and babel-jest
- add coverage/ directory to gitignore
- add babel-jest@23 for Babel 6 support
- and configure it for .js files due to a jest bug
- add .babelrc to configure babel-jest
- can't use .babelrc.js as babel-jest@23 doesn't support it
- jestjs/jest#5324
- can't use .babelrc for babel-loader (have to duplicate config)
because babel-loader@6 has some bugs with it and babel-loader@7
doesn't support webpack@1
- babel/babel-loader#552
(deps): add jest, babel-jest, and canvas-prebuilt to devDeps
- add canvas-prebuilt@1 to support jest's jsdom v11
- canvas is used by jsdom for, well, canvas interactions
- so it's not just handling one single square in the middle, make the shape more complex to ensure algorithm works properly
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 1
Lines ? 37
Branches ? 8
=======================================
Hits ? 37
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
test/index.spec.js
Outdated
| canvas.height = 1000 | ||
|
|
||
| trimCanvas(canvas) | ||
| // shouldn't it be 0? |
There was a problem hiding this comment.
might want to have scanX / scanY return -1 instead of null to fix this, though that might cause some bugs and not actually work as well
That might cause a breaking change though if anyone is relying on this 1x1 behavior, and given how long it's been since this library has had a release, reliance on buggy behavior might certainly exist (though no one's reported it as a bug either fwiw).
There was a problem hiding this comment.
okkkkk so changing them to return -1 causes Node to print some gc errors and then just crash.... I thought getImageData(-1, -1, -1, -1) might error hard, but didn't think it would cause Node to crash 🤯. I'm guessing node-canvas has some bug or something which makes it crash instead of throwing an invalid args error...
So I guess the only real solution would be to do some non-trivial refactoring to get cropXDiff and cropYDiff to be 0 instead of 1 -- because apparently null - null === 0 in JS.... I'm guessing this might cause some type-check issues when moving to TS. Maybe we'll refactor during the move to TS instead? TS refactor will be a minor release too, which can include breaking changes for the 0.x.x line.
There was a problem hiding this comment.
annnd cropXDiff and cropYDiff can't be 0, because that causes getImageData's IndexSizeError...
So we'd have to refactor some more so that it skips the getImageData call if if there's a null (or maybe -1 instead) return. It'd have to skip putImageData too and maybe clearRect too, so maybe it should just have it's own branched logic 😕 That would basically just be:
if (cropTop === null) {
canvas.width = 0
canvas.height = 0
return canvas
}that might interfere with the branching logic in #7 also
There was a problem hiding this comment.
Yea we'd have to add a case for if one is cloning (#7) here too. Alternatively, we could change #7 to just create a full clone at the top and then modify and return it as if it were the old canvas 🤔
In any case, not going to make changes in this PR / version line, might do so in the TS refactor which would also allow for a potential breaking change (that might not actually be breaking and may only impact a tiny amount of users).
- currently returns a 1x1 canvas... should probably be 0x0, but that
requires some refactoring and may be a breaking change if anyone
is relying on that behavior
- reliance on that behavior may certainly be possible given how long
this library has gone without a release, though no one's reported
that as a bug either
- it's a pretty big edge case, so I'm not sure that it matters much
either way tbh
- also, this gets us to 100% code coverage!!
- change CI to output and upload coverage reports - (docs): add dat slick coverage: 100% badge to README
- standardize w/ the rest of my libraries and better parity between test/ and src/
- none of these variables are ever re-assigned
- and new standard linter versions error on that
- I wrote this a while ago when ES6 was new and just used `let` for
everything instead of `var` at the time. Now ofc I use `const`
everywhere and `let` when needed
- also replace `var` in for loops with block-scoped `let`
- change docs to use const as well
- basically the same one I use in all my other libraries - I believe it's an old version of Node output from gitignore.io iirc - it has comments unlike the old one, so that's much nicer for those who aren't totally sure what each entry is for
agilgur5
changed the title
Resolves the one to-do I had for #7 , which was to add tests first to have more confidence that we won't break anything when changing a library that hasn't been changed in ~3.5 years.
Also moved
index.jsintosrc/in this PR instead of in #8 as thetest/directory is only created in this one (they were originally one big PR until I split them apart). Moved the refactor commit from #7 here as well so it goes along with the other move and because the tests here give us certainty that nothing has been impacted by the tiny refactor.This went a lot faster due to some of the learnings I had, mostly around compatibility issues, when adding tests for
react-signature-canvasin agilgur5/react-signature-canvas#33 and agilgur5/react-signature-canvas#34 . For instance, ran into the samebabel/babel-jest/babel-loader/.babelrcissues as there due to old Babel 6 and babel-loader 6 usage here, as well asIndexSizeError: The source width is 0(but changed the tests so that the latter wasn't used anyway).I also used
jesthere and notavadue to some of those learnings and because I now usejestvirtually everywhere and it, it's good built-in functionality, config options, community etc have most certainly grown on me.The one issue, similar to
react-signature-canvas, is that tests take quite a bit of time to run. Having usedjestmore nowadays, it seems like it's just the initialization time that has increased significantly, and, as far as I can tell, this seems to be due to the usage ofnode-canvas.In a weird turn of events, it seems like
@jest-runner/electron, which runs a full browser, is much faster than usingjsdomwithnode-canvas(based on my learnings in agilgur5/physijs-webpack#15 where it ran surprisingly very fast). But while it runs faster locally, on CI thenode-canvasversion runs faster... 😕I think it's better to have it run faster locally than on CI because you run tests locally much more often than on CI, but the differences are not huge, especially when factoring in
--watchmode, so I've just left it withnode-canvasfor now. For #7, I'll also have to add anode-canvastest specifically (ontestEnvironment: 'node') too, so probably makes sense to leave as is.