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