On Aug 12, 2015, at 7:55 AM, Kumar Srinivasan <kumar.x.srinivasan@oracle.com> wrote:
Henry,
Generally looks good here are some comments, on my initial pass, I am not fully done with args.c I will look at this some more later today or tomorrow.
4.) expectingNoDash is expectingMain right ? if so I would rename it so.
Not really, it is expecting a argument without dash, such as -cp or -classpath.
TestHelper.java:
Why add findInOutput method ? this seems to be identical to "matches" at line 606, which does exactly that.
To return a string, this is used when trying to match a pattern and get information from that line, I used to get the starting index for an argument in verifyOptions. Guess I can remove that now as each use of verifyOptions all starts from 1.
ArgFileSyntax.java Nice, but I think the creation and storage of test case can be simplified, basically loadCases(), also please avoid Xbootclasspath. The trouble with these tests which operate loops, are very painful to debug and isolate a failing case, on the night before GA.
I wrote that because I start with testng as a DataProvider, we can change that.
Can we do something like this.....
Map<String, List<String>> tests = new ArrayList<>(); tests.put("testing quotes", Array.asList({{...}, {....}}); tests.add("no recurse expansion", Array.asList({{...}, {....}});
The execute each test, with a description of what the test is doing. This way if a test fails its easy to zero in on the failing test.
verifyOutput will print out what line is not matching, so it’s pretty easy to identify which case failed. Cheers, Henry