test: remove TODO comment #1820
Conversation
|
ref: #264 |
| // TODO: randomly pick -r or --require | ||
| option += '-r ' + preload + ' '; | ||
| preloads.forEach(function(preload) { | ||
| var optionString = ['-r', '--require'][Math.floor(Math.random()*2)]; |
There was a problem hiding this comment.
Is there a way to test both, that way we don't introduce randomness into tests?
There was a problem hiding this comment.
I don't like the random either but decided that I should stick to the TODO recommendation at least as a first pass.
I thought about running each test twice, once with -r and once with --require, but that misses the edge case of using both.
I think the best thing to do might be to put these tests back they way they were (using -r always) and just add two more tests: One that uses --require and one that uses both.
There was a problem hiding this comment.
I think the best thing to do might be to put these tests back they way they were (using -r always) and just add two more tests: One that uses --require and one that uses both.
Would that work for you, @brendanashworth?
There was a problem hiding this comment.
IMO I don't think we should bulk it up to three tests if we can keep it in one - they don't really test very different functionality.
Sorry, what do you mean "edge case of using both"?
There was a problem hiding this comment.
Wrap the preloads.forEach in
['-r', '--require', '-r --require'].forEach(function(optionString) {
// preloads.forEach ... etc
})and call it a day?
The test itself should be fast, so I don't see why this isn't worth doing?
There was a problem hiding this comment.
@brendanashworth The edge case I was thinking of is node -r module1 --require module2. Thinking about it now, that can probably just be dropped. We don't test combinations of other command line flags--they just get tested individually.
So maybe just go with a simplified version of @dcousens's suggestion and wrap it in [-r', '--require'].forEach()`?
|
I pushed a new commit that runs each test twice, once with each flag. No randomness. No mixed use of both flags at the same time, but that's not in the existing tests either, so this is still an improvement. And probably no need to go down the slippery slope of flag combinations (not to mention ordering) unless/until an actual bug is discovered. |
|
I agree that this [edit: as suggested in todo] kind of randomness doesn't belong in tests but if we really want to start testing option parsing I think that should be done somewhere else (keen on growing cctests for instance). |
| if (err) throw err; | ||
| assert.equal(stdout, 'A\nB\n'); | ||
| }); | ||
| ['-r', '--require'].forEach(function (flag) { |
There was a problem hiding this comment.
minute-style: no space in function (flag), so function(flag)
|
Besides the style nitpick this LGTM. Thanks! |
|
@jbergstroem perhaps that could be added as part of #1804? |
|
@brendanashworth yeah. I don't see why we should waste time testing the same thing twice in here, is all. |
|
@jbergstroem from what I can see, it does increase the test run time by ~2 seconds, so your concerns are well placed. Do we know what is done for other things like this? |
|
@brendanashworth What do you mean by 'like'? In this case, my opinion is that we shouldn't test the same thing twice. If we want to make sure that the flags does the same thing, I suggested it be added to #1804 (just as you said :). |
|
@Trott do you object to closing this for now with the goal of later testing it in the C++ tests? |
|
How about I remove the loop and basically leave the tests as they were before (so we're not testing the same things with |
|
@Trott SGTM :) |
Trott
force-pushed
the
preload-long-short
branch
2 times, most recently
from
5ebc66e
to
bedf090
Compare
Trott
changed the title
The comment suggests adding randomness to the test suite.
|
OK, updated the commit so it's just removing the comment. Also updated the name of the pull request to better reflect what is now happening. |
|
LGTM |
The comment suggests adding randomness to the test suite. PR-URL: #1820 Reviewed-By: Johan Bergström <[email protected]>
|
Thanks, landed in d9ddd7d. |
Implement task from TODO comment.