Add Tests w/ 100% Code Coverage #21
Conversation
- add 1 simple test that just runs the persist function and ensures
it doesn't persist anything if an action hasn't been used
- change test script to use jest, and move prev test to test:pub
- (ci): change CI to run test and test:pub
- (deps): add @types/jest to devDeps
- also add reference in spec so that the types are loaded properly
- not sure why the reference is needed, but it would error without
- and plain `import`ing jest or @types/jest would also error
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 2
Lines ? 44
Branches ? 13
=======================================
Hits ? 44
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
agilgur5
changed the title
|
Updated the PR to ignore the
I also changed the And for some reason Travis CI checks aren't appearing in the PR GitHub UI right now (possibly because of bugs with general release of Actions?), so had to manually check to see that Node v8.9 or NPM v5.6 was causing some CI failures. Upgraded to Node v10 Active LTS and problems solved and CI passes -- all green! |
|
Decided I might as well see what I can do about adding a Node test, and turns out it's actually not as complicated as I thought because Jest does support changing I would've preferred having the Node test in the same file as the other tests, but unfortunately Jest doesn't support setting |
agilgur5
changed the title
- add one action in order to trigger onSnapshot
- add tests for no options, whitelist, and blacklist
- add comment about shallow cloning snapshot being required due to
non-configurable properties
- this code was borrowed from a gist, so I didn't actually know
why it was necessary until I tried running the test without
shallow cloning
- previously a bug made it so you couldn't set it to false
- this option is an edge case that's mostly useful for certain storage
engines like WebSQL, IndexedDB, etc that can actually handle
non-string data
- for localStorage, objects just get stored as '[object Object]' if
they're not jsonified
- might want to throw on jsonify = false and localStorage engine?
- gitignore coverage/ directory
- ignore asyncLocalStorage for now
- internally we only use getItem and setItem -- the other methods
are defined for strictly for type purposes
- maybe keep the file in coverage but ignore the unused methods?
- (deps): update tsdx to support custom jest configs
- instead of the whole file, just clear and removeItem, as those two
aren't currently used internally
- have to use this funky style comment-in-the-middle-of-function-
declaration for now as it won't ignore the func otherwise,
possibly because of how it gets transpiled down to ES5
- invalid usage of localStorage functions also isn't done internally,
so ignore the catch / Promise.reject block as well
- change CI to output and upload coverage reports - (docs): add coverage badge to README
- this is pretty critical functionality I forgot to add a test for... thanks to code coverage for pointing this out to me!
- <name>F makes it more explicit that we're referencing a fixture in the code, similar to e.g. TypeScript I<name> for interfaces
- just better organization and separation of concerns / test suites - might eventually split these out into separate files
- Node 8 Maintenance LTS is EOL in Jan 2020 - this also updates NPM to v6.9.0 (from v5.6.0) - and fixes whatever bug I was getting in https://travis-ci.org/agilgur5/mst-persist/builds/613842662
- add node test to reach 100% code coverage! - would prefer this test be in the same file as the others, but unfortunately jest only allows setting jest-environment on a per file basis, not on a per test or per test suite basis :/
Pull Request
Fixes #4
Process to get to completion
Was originally planning to get this done last week, but immediately hit bugs when I got
Cannot find name "describe"TypeScript errors. Adding@types/jestdid not resolve the issue. Earlier today I fixed the VSCode TS errors by adding animport @types/jestto the top, but thents-jesterrored out on that. Eventually fixed both by adding a/// <reference types="@types/jest" />at the top instead.I'm not sure why a reference declaration is necessary if TS is supposed to auto-import from
@types... Every SO Q&A I see says it should work past TS 2.1, and I'm using TS ^3.5.2...I also ran into another issue immediately after that, which was that I couldn't get tests to run from a
tests/directory because apparently Jest only checks thetest/(singular) directory 😕Was mostly smooth sailing writing tests after that, especially as
jsdomactually has alocalStorageimplementation built-in (andjesthas ajsdomintegration built-in... though it would probably make for a fatest env/faster tests to just polyfilllocalStoragein isolation).I did run into #18 while using the
localStorageimplementation immediately when I tried running the tests, so I actually had to fix that in the process 😅 . See the PR to fix that in #22Also added a test for #19 in this PR as well.
So that means this PR is dependent on #22 and #19 being merged before it can be merged. Locally I got all the tests to run, but this PR is split off to only have the tests related code.
When I got to adding code coverage, I noticed the AsyncLocalStorage
clearandremoveItemfunctions were causing a lot of lost coverage, but they're actually not used internally, they're more for consistency and typing (and might later be exposed externally). Had to update TSDX to getjest.config.jssupport so that I could add an ignore forasyncLocalStorage.ts. Was confused for a bit why myjest.config.jsdidn't work as I saw it was supported, but apparently this was a fairly recent feature.Then decided not to ignore the whole file, but just the two functions instead so that only the parts that need to be ignored are. Ran into some issues with ignoring coverage for ES6 class functions that seem related to gotwarlost/istanbul#445 (and I guess the istanbul org version still has the same issue -- the old repo should really be archived to avoid confusion). The workarounds listed there fixed it, but required some awkwardly placed comments.
EDIT: output definitely seems related to that issue. the awkwardly placed comments result in:
so I guess that makes sense why it has to go in between the name and parens then until this functionality is fixed.
Future Work
jsonifyis set tofalseandlocalStorageis the storage engine chosen? (bc it will save as'[object Object]'and that's almost certainly not intentional usage). We don't generally throw on improper usage of the Storage Engine though, and just put the responsibility on the user to know how their chosen engine works (e.g. could similarly error ifAsyncStorageis chosen andjsonifyisfalse-- that starts digging a rabbit hole of checking all sorts of engines. Also would need to know how to check if which storage it is without importing that storage). On the other hand,localStorageis the one and only default. 🤷♂localForageand possibly other storage engines, like maybeAsyncStorageand at least oneredux-persist-specific storage engine to ensure compatibility.Still missing coverage of the two.Promise.rejectinindex.tsand inasyncLocalStorage.ts. They're both kind of tricky edge cases to test, so maybe I should ignore them? The former requires writing a test to run specifically injest --env=nodeinstead ofjsdom(which means doing some hackish code to split where or when that test runs) and the latter requires doing an invalid operation onAsyncLocalStorage, which shouldn't happen in internal usage (so that would be part of a potential future AsyncLocalStorage unit test suite, so maybe it should indeed be ignored likeclearandremoveItem)EDIT:
Promise.rejectinasyncLocalStorage.tshas been ignored similar toclearandremoveItemper the reasoning above that it's not part of internal usage.EDIT2: A node test has been added for the edge case of
Promise.rejectinindex.ts.