Add Tests & 100% Code Coverage #34
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- add 1 simple test for existence of canvas and instance methods
- (pkg): test code shouldn't be in the package, just source
- currently preferring to keep source and tests co-located
- change test script to use jest, and move prev test to test:pub
- (ci): change CI to run test and test:pub
- configure jest and enzyme for testing usage
- make jest importable too
- 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 because babel-loader@6
has some bugs with it and @7 doesn't support webpack@1
- babel/babel-loader#552
- use jest instead of ava mainly because there's less to
configure / install (directly at least, still a lot indirectly),
it's popular / well-supported in React community, and supports
configuration outside of package.json
- see #33 for some more details
(deps): add jest, enzyme, and supporting deps to devDeps
- add babel-jest@23 for Babel 6 support
- and configure it for .js files due to a jest bug
- add canvas-prebuilt@1 to support jest's jsdom v11
- canvas is used by jsdom for, well, canvas interactions
- add enzyme-adapter-react-16 to configure Enzyme with React 16
- upgrade to React 16 in devDeps bc adapter-react-15 requires
react-test-renderer and no drawbacks in upgrading
- add describe as this is several tests in one - add a fixture of just a dot - add tests for toData, fromData, toDataURL, fromDataURL, isEmpty, and clear
- needed to add width & height to element as otherwise trimming would fail due to non-existent width & height - add initial and trimmed width & height as fixtures - tried using some dataURL fixtures but they would end up slightly different, so I suppose a bit of that is implementation-specific
- ignore test-utils/ directory in coverage - update gitignore with coverage/ directory - and fix missing newline
- change CI to output and upload coverage reports - (docs): add coverage badge to README
- that defaults, mounted initial props and options, and updated props
and options match
- took me a bit to figure out I should use `toMatchObject` and then
how to use `toMatchObject` properly
- add fixtures for props and split them into sigPad vs. rSC for
different uses
- better organization to combine related fixture data as one object - naming as <prefix>F makes it more clear that we're referencing a fixture in the code, similar to e.g. TypeScript I<suffix>
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 1
Lines ? 59
Branches ? 6
=======================================
Hits ? 59
Misses ? 0
Partials ? 0Continue to review full report at Codecov.
|
- clearOnResize and fixed/unfixed width & height should affect
resizing as expected
- also yea the tests run in parallel, but readability-wise it makes
more sense I think(?)
- add a window.resizeTo polyfill as jsdom doesn't implement it o.o
- and there doesn't seem to be an NPM package for this(?)
- noun / verb of describe and it for better readability - export it from Jest ES Module as well
- 100% statement coverage now woooooot!
- when false, canvas won't resize on window resize necessarily
- was set false in previous test
- this was found when branch coverage was lacking a bit on resizing,
now we're at ~93% and missing just one branch!
- the tests didn't error out though because without input size the
canvas size is 0 in these tests regardless of resize
- not a perfect test in that sense :/ :/
agilgur5
commented
Aug 4, 2019
There was a problem hiding this comment.
I think there are still some improvements to be made, but this is small enough that those might be premature without seeing how the codebase evolves. For now, I think this is good, and future iterations can improve. Several reviews, amendments, and refactors already made it in before I officially made the PR as well.
Some future things to think about:
- DPR test as per above comments
- make resize tests more reliable as per above comments
- splitting fixture data into multiple files with
import * as <prefix>F from '../test-utils/fixtures/<prefix>.js- map
test-utilsorfixturesetc to local scope so that one can `import ... from 'fixtures/....js' etc
- map
- splitting tests into multiple files? for this and a bit for fixtures, kind of depends on how strictly we're following 1-to-1 test files per source code
- convert more of the tests to BDD-style, might require nested describes for it to make sense?
- use
beforeEachso that nested describes don't causejestordering problems (would also make each test cleaner).
agilgur5
added
the
kind: internal
Repository owner
locked as resolved and limited conversation to collaborators
Mar 13, 2021
agilgur5
added
the
scope: tests
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #33. Also wrote through my progress, bugs/gotchas encountered, and links everywhere in #33 as well.
This might need a bit of iteration on code style.
It's also missing coverage for exactly 1 branch, which is the DPR check, the
|| 1specifically (though DPR is1already injsdom). I'm not sure if I want to add a test for it though as the code around DPR has always been iffy/problematic in upstreamsignature_padand occasionally here too.DPR being
1already points to another potential review point, which is thatcanvas.widthandcanvas.heightare0if they're not set as there's no CSS to make it a percentage of some wrapper, etc, so this made the change size tests accidentally succeed even whenclearOnResizewasfalse(i.e. not necessarily that reliable a test).Anyway, this took a lot longer than expected for a few reasons, so I might not review / merge / iterate on this for a bit, we'll see.