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