Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: remove TODO comment #1820

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 28, 2015

Implement task from TODO comment.

@mscdex mscdex added the test Issues and PRs related to the tests. label May 28, 2015
@Trott
Copy link
Member Author

Trott commented May 28, 2015

ref: #264

// TODO: randomly pick -r or --require
option += '-r ' + preload + ' ';
preloads.forEach(function(preload) {
var optionString = ['-r', '--require'][Math.floor(Math.random()*2)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to test both, that way we don't introduce randomness into tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()`?

@Trott
Copy link
Member Author

Trott commented May 28, 2015

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.

@jbergstroem
Copy link
Member

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minute-style: no space in function (flag), so function(flag)

@brendanashworth
Copy link
Contributor

Besides the style nitpick this LGTM. Thanks!

@brendanashworth
Copy link
Contributor

@jbergstroem perhaps that could be added as part of #1804?

@jbergstroem
Copy link
Member

@brendanashworth yeah. I don't see why we should waste time testing the same thing twice in here, is all.

@brendanashworth
Copy link
Contributor

@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?

@jbergstroem
Copy link
Member

@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 :).

@brendanashworth
Copy link
Contributor

@Trott do you object to closing this for now with the goal of later testing it in the C++ tests?

@Trott
Copy link
Member Author

Trott commented Jun 2, 2015

How about I remove the loop and basically leave the tests as they were before (so we're not testing the same things with -r and --require twice), but remove the TODO comment about randomizing the flag? So the pull request will just remove a comment that is suggesting introducing randomness into the test.

@jbergstroem
Copy link
Member

@Trott SGTM :)

@Trott Trott force-pushed the preload-long-short branch 2 times, most recently from 5ebc66e to bedf090 Compare June 2, 2015 05:35
@Trott Trott changed the title test: use both -r and --require in preload tests test: remove TODO Jun 2, 2015
@Trott Trott changed the title test: remove TODO test: remove TODO comment Jun 2, 2015
The comment suggests adding randomness to the test suite.
@Trott
Copy link
Member Author

Trott commented Jun 2, 2015

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.

@brendanashworth
Copy link
Contributor

r=@jbergstroem

@jbergstroem
Copy link
Member

LGTM

brendanashworth pushed a commit that referenced this pull request Jun 9, 2015
The comment suggests adding randomness to the test suite.

PR-URL: #1820
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@brendanashworth
Copy link
Contributor

Thanks, landed in d9ddd7d.

@rvagg rvagg mentioned this pull request Jun 11, 2015
@Trott Trott deleted the preload-long-short branch October 14, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants