RFR: JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching [v2]

[Jonathan Gibbons]

On Fri, 1 Oct 2021 12:04:53 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

> Unrelated to this PR. Every time I see these lines in JavadocTester, I cannot help thinking that we could use more specific APIs:
> 
> ```
>     public static final String FS = System.getProperty("file.separator");
>     public static final String PS = System.getProperty("path.separator");
>     public static final String NL = System.getProperty("line.separator");
>     public static final String thisRelease = System.getProperty("java.specification.version");
> ```
> 
> The above lines can be translated into:
> 
> ```
>     public static final String FS = java.io.File.separator;
>     public static final String PS = java.io.File.pathSeparator;
>     public static final String NL = System.lineSeparator();
>     public static final String thisRelease = String.valueOf(Runtime.version().feature());
> ```
> 
> I note that neither `PS` nor `thisRelease` seem to be currently used.

I dislike importing `File`, to help catch accidental use of `File` when `Path` would be better.
You may be right that `PS` and `thisRelease` may not be used, but I'd prefer to leave them in, just in case.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 923:
> 
>> 921:      * Configuration can be done with a series of chained method calls.
>> 922:      * Checks can be specified as either literal strings or regular expressions.
>> 923:      */
> 
> Suggestion:
> 
>     /**
>      * A flexible tool for checking the content of generated files and output streams.
>      *
>      * Configure with a series of chained method calls, specifying target strings
>      * as either literal strings or regular expressions.
>      */

I prefer to keep `checker` instead of `tool`.   I'll consider the others.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 938:
> 
>> 936:                 return new Range(start, end);
>> 937:             }
>> 938:             boolean overlaps(Range other) {
> 
> The `overlaps` method seems unused.

That sounds surprising; I'll check.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 953:
> 
>> 951:          * @param file the file
>> 952:          */
>> 953:         public OutputChecker(String file) {
> 
> Given JDK-8274172, consider using java.nio.file.Path instead of String here. Using Path for files and String for target strings reduces potential for confusion.

I'll investigate, but IIRC there is broad precedent for using `String file` to simplify the call site, avoiding boilerplate `Path.of`.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1030:
> 
>> 1028:                         .map(s -> "    " + toShortString(s))
>> 1029:                         .collect(Collectors.joining("\n")));
>> 1030:                 return this;
> 
> Shouldn't we print lines using the platform line separator?
> 
> Suggestion:
> 
>             if (name == null) {
>                 out.println("Skipping checks for:" + System.lineSeparator()
>                         + Stream.of(strings)
>                         .map(s -> "    " + toShortString(s))
>                         .collect(Collectors.joining(System.lineSeparator())));
>                 return this;
> 
> Alternatively, we could println individual lines:
> Suggestion:
> 
>             if (name == null) {
>                 out.println("Skipping checks for:");
>                 for (String s : strings) {
>                     out.println("    " + toShortString(s));
>                 }
>                 return this;
>             }

Will consider

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1063:
> 
>> 1061:          * Checks that there are no duplicate lines in the content.
>> 1062:          */
>> 1063:         public OutputChecker checkUnique() {
> 
> checkUnique seems to be neither used nor tested. As far as I can see, it was like that before this PR. Consider removing this method.

I'll check when/why it was added.  It must have been added for a reason.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1076:
> 
>> 1074:          *                if {@code false}, lines that do not match the pattern will be checked
>> 1075:          */
>> 1076:         public OutputChecker checkUnique(String pattern, boolean select ) {
> 
> Suggestion:
> 
>         public OutputChecker checkUnique(String pattern, boolean select) {

oops

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1079:
> 
>> 1077:             checking("checkUnique");
>> 1078:             Pattern filter = Pattern.compile(pattern);
>> 1079:             Matcher m = filter.matcher("");
> 
> A reusable matcher; nice.

Yeah, not sure it makes much difference, compared to allocating new ones.

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1167:
> 
>> 1165:          *
>> 1166:          * @param finder a function to find the next occurrence of an item starting at a given position
>> 1167:          * @param kind   the kind of the item ({@code "text"} or {@code "pattern:} to include in messages
> 
> Suggestion:
> 
>          * @param kind   the kind of the item ({@code "text"} or {@code "pattern"}) to include in messages

oops

> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1227:
> 
>> 1225: 
>> 1226:         private int getLineNumber(int pos) {
>> 1227:             Pattern p = Pattern.compile("\\R");
> 
> \R is a Unicode linebreak sequence, which includes more than \n and \r\n. Can \R give us a spurious line, in particular when analysing HTML output?

It seems better to be more tolerant than pedantic in this case.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5743