RFR: 7903659 Verify Problemlist contents
    Jonathan Gibbons 
    jjg at openjdk.org
       
    Wed Mar  6 23:36:03 UTC 2024
    
    
  
On Fri, 9 Feb 2024 20:07:20 GMT, Ludvig Janiuk <lujaniuk at openjdk.org> wrote:
> This change introduces the flag `--excludeverify` which will run additional checks on the problemlists passes through `-exclude`. If any of the checks fails, jtreg prints a helpful message and refuses to start running tests.
Very good for a first attempt.
Some minor suggestions given.
src/share/classes/com/sun/javatest/regtest/tool/ExcludeFileVerifier.java line 40:
> 38:                 if (lineIsComment(line.trim())) continue;
> 39:                 for(Check c : checks) {
> 40:                     if(!c.check(line.trim())) {
Code style issues.   
* Space between keyword and left-paren
* `{` at end of line, not beginning of next
src/share/classes/com/sun/javatest/regtest/tool/ExcludeFileVerifier.java line 46:
> 44:                         System.out.println("--------------");
> 45:                         System.out.println(line);
> 46:                         System.out.println("--------------");
Don't use `System.out` directly like this; pass in a `PrintWriter` to be used. 
The main `jtreg``Tool` class sets up two stream to be used, `out` and `err`.  
Generally, 
* `standard output` / `System.out` / `sysOut` / `stdOut` should be used for the expected output of specific options, like command-line help.
* `standard error` / `System.err` / `sysErr` / `stdErr` should be used for diagnostic output when issues are found performing any requested operation.
src/share/classes/com/sun/javatest/regtest/tool/ExcludeFileVerifier.java line 77:
> 75:     class LineFormatCheck extends Check {
> 76:         private static final String commalist = "([\\w-]+)(,[\\w-]+)*";
> 77:         private static final String p = "\\S+\\s+" + commalist + "\\s" + commalist + ".*";
Hmm, I see what you're doing, but generally it is better to use `Pattern.compile` for regular expressions.
src/share/classes/com/sun/javatest/regtest/tool/ExcludeFileVerifier.java line 83:
> 81: 
> 82:         public boolean check(String line) {
> 83:             return Pattern.matches(p, line);
Not a huge deal, but this will compile the regular expression every time it is called. It is better to compile the regex once, using `Pattern.compile` and then create a `Matcher` from the `Pattern`.
src/share/classes/com/sun/javatest/regtest/tool/Tool.java line 646:
> 644:         },
> 645: 
> 646:         new Option(NONE, MAIN, null, "-ve", "-verifyexclude") {
I might have gone for `--verify-exclude` but this is OK
-------------
Changes requested by jjg (Lead).
PR Review: https://git.openjdk.org/jtreg/pull/181#pullrequestreview-1921019450
PR Review Comment: https://git.openjdk.org/jtreg/pull/181#discussion_r1515275600
PR Review Comment: https://git.openjdk.org/jtreg/pull/181#discussion_r1515280491
PR Review Comment: https://git.openjdk.org/jtreg/pull/181#discussion_r1515281944
PR Review Comment: https://git.openjdk.org/jtreg/pull/181#discussion_r1515283266
PR Review Comment: https://git.openjdk.org/jtreg/pull/181#discussion_r1515284960
    
    
More information about the jtreg-dev
mailing list