Add tests for collection args with ranges. #44
Conversation
cmnbroad
changed the title
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
============================================
+ Coverage 71.36% 71.75% +0.38%
+ Complexity 504 498 -6
============================================
Files 21 21
Lines 2050 2064 +14
Branches 425 427 +2
============================================
+ Hits 1463 1481 +18
+ Misses 407 404 -3
+ Partials 180 179 -1
Continue to review full report at Codecov.
|
| {new String[]{"--intListArg", "-1"}}, | ||
| {new String[]{"--intListArg", "57"}}, | ||
| {new String[]{"--intListArg", "1", "--intListArg", "57"}}, | ||
| {new String[]{"--intListArg", "null" }}, |
There was a problem hiding this comment.
Is there any test for this if the collection does not have bounded arguments? Maybe this corresponds here too.
There was a problem hiding this comment.
Yes, see testClearDefaultValuesFromListArgumentAndAddNew.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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 = "goodBoundedCollectionArgs") | ||
| public void testGoodBoundedCollections(final String[] argv, final Integer[] expectedIntegers) throws Exception { |
There was a problem hiding this comment.
Why expectedIntegers is not int[]?
There was a problem hiding this comment.
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.
| optional = true, | ||
| minValue = 0, maxValue = 30, | ||
| minRecommendedValue = 10, maxRecommendedValue = 15) | ||
| public List<Integer> intListArg = new ArrayList<>(); |
There was a problem hiding this comment.
What will happen if the default list includes a value out of range? It will blow up? Maybe that also requires a test...
There was a problem hiding this comment.
And what will happen if there are default values and --intListArg null --intListArg 15 to clean the defaults?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I forgot that now is replace instead of append. Thanks for remind me.
There was a problem hiding this comment.
And thanks for the check of the initial value out of the range.
cmnbroad
force-pushed
the
cn_bounded_collections
branch
from
fb7a0e4
to
d27c612
Compare
| public void testResetListToNull() { | ||
| class ResetListToNull { | ||
| @Argument | ||
| public List<String> LIST = makeList("foo", "bar"); |
There was a problem hiding this comment.
I will use instead a numeric list to check that this is correct. That was one of my bugs (see #41) and maybe it is better to be sure that it is not broken also for collections.
| || !isInfinityOrMathematicalInteger(this.minValue) | ||
| || !isInfinityOrMathematicalInteger(this.maxRecommendedValue) | ||
| || !isInfinityOrMathematicalInteger(this.minRecommendedValue)) { | ||
| throw new CommandLineException.CommandLineParserInternalException( |
There was a problem hiding this comment.
Can the maximum (or minimum) value be infinity? If so, this will fail. I agree that users should not specify these values, but perhaps this error message should include the suggestion not to specify infinity.
| public void testBoundedCollectionIntializedOutOfRange() throws Exception { | ||
| @CommandLineProgramProperties( | ||
| summary = "tool with bounded collection argument with bad initial value", | ||
| oneLineSummary = "ool with bounded collection argument with bad initial value", |
| // Check recommended values | ||
| if (hasRecommendedRange() && isOutOfRange(minRecommendedValue, maxRecommendedValue, argumentDoubleValue)) { | ||
| final boolean outMinValue = minRecommendedValue != Double.NEGATIVE_INFINITY; | ||
| final boolean outMaxValue = maxRecommendedValue != Double.POSITIVE_INFINITY; |
There was a problem hiding this comment.
Do we need to check for NaN? This may be a matter of opinion...
Just noticed we don't have any tests for collections with ranges, but we should.