Henry,
<trimmed the cc: list>
I am still not complete, but here are some more comments and specifics.
args.c
1.
-static int firstAppArgIndex = -1;
+#define NOT_FOUND -1;
+static int firstAppArgIndex = NOT_FOUND;
2.
// Any @argument comes in here is an argument as is.
// Expansion should had done before checkArg called.
I think you are trying to say
// All arguments arrive here must be a launcher argument,
// ie. by now, all argfile expansions must have been performed.
3. void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) {
// No expansion for relaunch
The launcher might re-exec itself, this might happen under certain
circumstances on *nix, is the above logic to prevent subsequent
expansion on relaunch ? However when a re-exec occurs, the
expanded args will be sent to the second invocation of the launcher.
So what does "relaunch" mean ? in your comment ?
ArgFileSyntax.java:
usage of ArrayList vs. Arrays will simplify things.
// arg file content, expected options
static String[] testCaseArrays[][] = {
{ // empty file
{}, {}
},
...
public List<List<List<String>>> loadCases() {
List<List<List<String>>> rv = new ArrayList<>();
for (String[][] testCaseArray : testCaseArrays) {
List<List<String>> testCase = new ArrayList<>(2);
testCase.add(Arrays.asList(testCaseArray[0]));
testCase.add(Arrays.asList(testCaseArray[1]));
rv.add(testCase);
}
// long lines
.....
@Test
public void testSyntax() throws IOException {
List<List<List<String>>> allCases = loadCases();
for (List<List<String>> testCase : allCases) {
verifyParsing(testCase.get(0), testCase.get(1));
}
}
ArgsFileTest.java:
use case coverage looks great, still haven't finished with this
yet.
Kumar
On Aug 12, 2015, at 7:55 AM, Kumar Srinivasan <kumar.x.sriniva...@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
