RFR: JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching [v2]
Pavel Rappo
prappo at openjdk.java.net
Fri Oct 1 15:54:43 UTC 2021
On Wed, 29 Sep 2021 03:45:39 GMT, Jonathan Gibbons <jjg at 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
More information about the javadoc-dev
mailing list