Complete lib/query.js tests
#186
Conversation
The else path will never be taken because callbackToPromise which wraps this function call will ensure there is always a `done` function.
test/query.test.js
Outdated
|
|
||
| var Query = require('../lib/query'); | ||
|
|
||
| describe('Query', function() { |
There was a problem hiding this comment.
I get the sense that Query is not a public API, so these tests may be a hindrance during refactoring. What do you think about verifying this behavior through Table#select?
There was a problem hiding this comment.
I am testing most things in select because I see at as likely being the public API.
The handful of error states I'm addressing here are actually impossible to reach via select because select does its own validation and parameter filtering and error catching before passing the query directly to select. So its not possible to have 100% code coverage with out these unit tests. See lib/table:49 Should I remove them and add ignore? Or add a note?
I could move the validates cellFormat params but that feels like an okay low level unit test to me.
There was a problem hiding this comment.
If Query is truly private, then any code paths that can't be reached in production are dead.
We could certainly merge the tests even if that's true. They're correct, after all. My only concern is that, in the absence of documentation, the presence of tests may lead others to believe that the code they exercise is important.
@kasra We believe that Query is private. Could you confirm?
There was a problem hiding this comment.
Yep Query is intended to be private, so would prefer to not add tests specifically for it. I think it would be reasonable to remove the extra error checking in the Query constructor if they're not reachable, but let's also update the comment for the Query constructor to document that it expects the caller to first validate params by calling Query.validateParams.
There was a problem hiding this comment.
I completely removed and added comments about all of the unreachable error conditions which allowed me to completely remove the query.test.js file.
test/select.test.js
Outdated
| iterationCounter++; | ||
| fetchNextPage(); | ||
| }).then(function() { | ||
| expect(iterationCounter).toBe(2); |
There was a problem hiding this comment.
Sorry for the noisy CI report; we probably don't need to lint across versions of Node.js. This patch ought to help.
test/query.test.js
Outdated
|
|
||
| var Query = require('../lib/query'); | ||
|
|
||
| describe('Query', function() { |
There was a problem hiding this comment.
Yep Query is intended to be private, so would prefer to not add tests specifically for it. I think it would be reasonable to remove the extra error checking in the Query constructor if they're not reachable, but let's also update the comment for the Query constructor to document that it expects the caller to first validate params by calling Query.validateParams.
callbackToPromise ensures there will always be a `done` function.
There is error checking for a plain object at `lib/table.js:63` and `Query` is meant to be a private interface so this code path will never be hit.
It is not used because validation id done with Query.validateParams in `lib/table.js`.
This checking is done in `lib/table.js` so it isn't needed here.
Move use of cellFormat into select test so that query.test.js can be remove.
| if (done) { | ||
| done(null); | ||
| } | ||
| done(null); |
There was a problem hiding this comment.
The check on line 68 and corresponding error message on line 69 says it's valid for done to be undefined, which would make this line crash. Is that not possible because of callbackToPromise? If so, we should update the check on line 68-69
There was a problem hiding this comment.
The end use of eachPage can allow done to be undefined and in that case it will be defined by this.eachPage = callbackToPromise(eachPage, this, 1); on query line 30. So it is an error message that make sense from an end user perspective.
There was a problem hiding this comment.
Ah gotcha, thanks for the explanation
selecttests which actually use the query's functionalitycallbackToPromise