Skip to content

Add tests for collection args with ranges. #44

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add tests for collection args with ranges. #44

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f46a61
Add tests for bounded collection args.
cmnbroad Feb 27, 2017
d27c612
Add support and tests for initial value out of range.
cmnbroad Mar 1, 2017
01a7a8c
Add test for simple reset of unbounded list using null
cmnbroad Mar 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,56 @@ public static List<String> makeList(final String... list) {
return result;
}

@CommandLineProgramProperties(
summary = "tool with bounded collection argument",
oneLineSummary = "bounded collection argumen",
programGroup = TestProgramGroup.class
)
public class BoundedCollection {
@Argument(doc = "Integer collection in the range [0, 30]",
optional = true,
minValue = 0, maxValue = 30,
minRecommendedValue = 10, maxRecommendedValue = 15)
public List<Integer> intListArg = new ArrayList<>();
Copy link
Contributor

@magicDGS magicDGS Feb 28, 2017

Choose a reason for hiding this comment

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

What will happen if the default list includes a value out of range? It will blow up? Maybe that also requires a test...

Copy link
Contributor

@magicDGS magicDGS Feb 28, 2017

Choose a reason for hiding this comment

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

And what will happen if there are default values and --intListArg null --intListArg 15 to clean the defaults?

Copy link
Collaborator Author

@cmnbroad cmnbroad Mar 1, 2017

Choose a reason for hiding this comment

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

For the first one, there is neither tests nor code to check that the initial value is within range. I added code that throws an internal exception if the initial value is out of range, for both scalar and collection, and tests.

The second one is already illegal. Since the default behavior of this parser for collections is "replace" rather than "append", the parser already enforces that if you pass "null" as a value for a collection, you cannot pass any other values for that arg. If you want to reset the list to a single value of 15, then you just say "--intListArg 15." Finally, I believe we already have a test to validate that "null" is not a valid value for something with a range (since null is always outside the range); I added an additional test to verify that for collections as well.

Copy link
Contributor

@magicDGS magicDGS Mar 2, 2017

Choose a reason for hiding this comment

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

Oh, I forgot that now is replace instead of append. Thanks for remind me.

Copy link
Contributor

@magicDGS magicDGS Mar 2, 2017

Choose a reason for hiding this comment

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

And thanks for the check of the initial value out of the range.

}

@DataProvider(name="goodBoundedCollectionArgs")
public Object[][] goodBoundedCollection() {
return new Object[][]{
{new String[]{"--intListArg", "1"}, new Integer[]{ 1 } },
{new String[]{"--intListArg", "19", "--intListArg", "2"}, new Integer[]{ 19, 2 } },
{new String[]{"--intListArg", "0", "--intListArg", "18", "--intListArg", "27" }, new Integer[]{ 0, 18, 27 } },
};
}

@Test(dataProvider = "goodBoundedCollectionArgs")
public void testGoodBoundedCollections(final String[] argv, final Integer[] expectedIntegers) throws Exception {
Copy link
Contributor

@magicDGS magicDGS Feb 28, 2017

Choose a reason for hiding this comment

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

Why expectedIntegers is not int[]?

Copy link
Collaborator Author

@cmnbroad cmnbroad Mar 1, 2017

Choose a reason for hiding this comment

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

Only because Integer is the type of the value in the argument, so it makes the Assert overload unambiguous. I did remove the unnecessary .getInt calls from the Asserts though.

Copy link
Contributor

@magicDGS magicDGS Mar 2, 2017

Choose a reason for hiding this comment

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

Ok, I understand it now.

final BoundedCollection o = new BoundedCollection();
final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
Assert.assertTrue(clp.parseArguments(System.err, argv));
Assert.assertEquals(expectedIntegers.length, o.intListArg.size());
for (int i = 0; i < expectedIntegers.length; i++) {
Assert.assertEquals(o.intListArg.get(i).intValue(), expectedIntegers[i].intValue());
}
}

@DataProvider(name="badBoundedCollectionArgs")
public Object[][] badBoundedCollection() {
return new Object[][]{
{new String[]{"--intListArg", "-1"}},
{new String[]{"--intListArg", "57"}},
{new String[]{"--intListArg", "1", "--intListArg", "57"}},
{new String[]{"--intListArg", "null" }},
Copy link
Contributor

@magicDGS magicDGS Feb 28, 2017

Choose a reason for hiding this comment

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

Is there any test for this if the collection does not have bounded arguments? Maybe this corresponds here too.

Copy link
Collaborator Author

@cmnbroad cmnbroad Mar 1, 2017

Choose a reason for hiding this comment

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

Yes, see testClearDefaultValuesFromListArgumentAndAddNew.

Copy link
Contributor

@magicDGS magicDGS Mar 2, 2017

Choose a reason for hiding this comment

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

So it is illegal to provide null always? I mean, if the intListArg in this case does not have boundaries, is it forbiden to provide --intListArg null (without any other argument)? And the same case for any other object collection....

Copy link
Collaborator Author

@cmnbroad cmnbroad Mar 2, 2017

Choose a reason for hiding this comment

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

No. If there is no range specified, its perfectly legal to use null to reset the collection (as long as there are no other arguments). But I think you're right that there is no positive test for this, only the negative test to verify that you can't provide other values along with null. I'll add one. At some point I really want to refactor these tests since its really hard to tell whats covered and whats not.

};
}

@Test(dataProvider = "badBoundedCollectionArgs", expectedExceptions = CommandLineException.OutOfRangeArgumentValue.class)
public void testBadBoundedCollections(final String[] argv) throws Exception {
final BoundedCollection o = new BoundedCollection();
final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
Assert.assertTrue(clp.parseArguments(System.err, argv));
}

@Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
public void testHiddenRequiredArgumentThrowException() throws Exception {
@CommandLineProgramProperties(summary = "tool with required and hidden argument",
Expand Down