Build: make sure to test all callback args #114
Conversation
erikkemperman
force-pushed
the
test-callback-args
branch
from
c5e2d8d to
f4cb6f6
Compare
|
@erikkemperman Thank you for pointing out. I agree and I’ll amend the added test codes in #75 and #113 according to the principle. |
|
@sttk It seems that I inadvertedly managed to decrease the coverage here and there, but I haven't figured out yet how exactly. Should I worry about that, do you think? |
phated
left a comment
phated
left a comment
There was a problem hiding this comment.
I think I know the answer to all these questions (just following established patterns?) but I think you should update them to my suggestion unless you have another reason.
| function cb(err, stdout) { | ||
| expect(err).toExist(); | ||
| function cb(err, stdout, stderr) { | ||
| expect(err).toNotEqual(null); |
There was a problem hiding this comment.
I prefer toExist, why did you change to toNotEqual(null)?
| .run(cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toNotEqual(null); |
There was a problem hiding this comment.
I prefer toExist, why did you change to toNotEqual(null)?
|
|
||
| function cb(err, stdout) { | ||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
|
|
||
| function cb(err, stdout) { | ||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
|
|
||
| function cb(err, stdout) { | ||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
| child.exec('node ' + __dirname + '/fixtures/logging.js -LLLL', cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
| child.exec('node ' + __dirname + '/fixtures/logging.js', cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
| child.exec('node ' + __dirname + '/fixtures/logging.js -LLL', cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
| child.exec('node ' + __dirname + '/fixtures/logging.js -LL', cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
| child.exec('node ' + __dirname + '/fixtures/logging.js -L', cb); | ||
|
|
||
| function cb(err, stdout, stderr) { | ||
| expect(err).toEqual(null); |
There was a problem hiding this comment.
I prefer toNotExist, why did you choose to use toEqual(null)?
|
Yeah, I just used the established pattern. You're right though, but perhaps we could address that separately? Otherwise this will have to wait until I get back from holiday in about 2 weeks. |
|
@erikkemperman Yeah, I think that's fair. |
|
Merged as 0abb70d (cherry-pick to drop the appveyor changes) |
3 participants
Playing around with an idea (not sure yet if I want to submit that) I found it convenient to always assert the outputs of all the streams, this helps narrow things down quickly when tests fail.
Note: If you guys agree with this principle, I guess the open PRs #75 and #113 might have to be amended accordingly. Let me know if I can help out.