On Wed, 29 Sep 2021 03:45:39 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>> Please review a moderately simple improvement for `JavadocTester` and a
>> related new test.
>>
>> A new `OutputChecker` class is introduced that mostly supersedes the
>> existing methods to check the output generated by javadoc and the standard
>> doclet. A self-imposed restriction is that no existing tests are modified.
>>
>> The new class can be used to check files generated by the doclet and the
>> streams written by the tool. It can be configured to check for ordered
>> output or not, overlapping output, and complete coverage, and can search for
>> literal strings and regular expressions.
>>
>> There is a corresponding new test which is a non-standard use of
>> `JavadocTester`, since it is designed to test `JavadocTester` itself, and
>> not javadoc or the doclet. (Quis custodiet ipsos custodes?) Various
>> methods are overridden so that the operation of the underlying methods can
>> be checked.
>>
>> Although it is a goal to NOT modify the code of any existing tests, it turns
>> out to be reasonable to adapt some of the existing `check...` methods to use
>> the new `OutputChecker`. All javadoc tests pass, both locally and on all
>> standard platforms. Many/most uses of the existing `checkOutput` method
>> provide "ordered" strings, and are candidates to use the new ordered check.
>> But enough uses are _not_ ordered, so it is not reasonable to change the
>> default at this time. It is noted as a TODO to examine the appropriate test
>> cases, so that we can decide whether to fix those tests and change the
>> default.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains two commits:
>
> - Merge with upstream/master
> - JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered
> output matching
Thanks a lot for doing this, Jon. Do you think you could also address closely
related JDK-8270836 in this PR? Meanwhile, I'll keep reviewing the code.
test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 540:
> 538: checking("checkOutput");
> 539: // Find string in file's contents
> 540: boolean isFound = findString(fileString, stringToFind);
Since you deleted checkOutput, which was the only user of `findString(String,
String)`, consider also deleting `findString(Stirng, String)`.
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.
*/
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.
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.
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;
}
test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1053:
> 1051: .collect(Collectors.joining("\n")));
> 1052: return this;
> 1053: }
Same question as for L1025-L1030.
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.
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) {
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.
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
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?
test/langtools/jdk/javadoc/testJavadocTester/TestJavadocTester.java line 59:
> 57: * tests have failed.
> 58: */
> 59: public class TestJavadocTester extends JavadocTester {
+1 for testing a testing tool.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5743
