Closed apihlaja closed 8 years ago
Thank you @apihlaja, nice work!
@apihlaja - it seems pretty good. You did an awesome job here.
Thanks, I didnt expect it to be merged right away.
There is still some details to think about, especially pending tests. I was not sure if that step order thing is buggy or if I dont understand it correctly. Maybe @eberhara could look into it. Other pending test is basically a comment about clumsy looking internal API, I think this.connectionUri || this.getConnectionUri()
should be in getConnectionUri
.
Some other random thoughts:
Like @neekfenwick was suspecting, currently tests are not really ran at all. The only test tries to hit database, and sometimes its quick enough to do it. But
it
is missing callback so test suite shuts down immediately without waiting for callbacks.Since there is also open issue #3 about unit testing, I decided to play around a bit when evaluating the module. I´m interested extending API a little bit but not brave enough to do it before regression tests exist. So, I fixed orginal test, added
npm run istanbul
to monitor coverage.. And wrote tests for everything I could. Istanbul shows 83 % branch coverage but those are mostly null checks etc. I found one potential bug, I think order check is not working correctly. At least, for the first step because counting starts from 0 which evals to false.It took me some time to figure out orginal test actually uses database. Now its renamed
*-itest.js
ie. integration test to highlight that its not unit-test, also makes it easier to filter it out.