RFR: 8273544: Increase test coverage for snippets [v3]

[Jonathan Gibbons]

On Fri, 19 Nov 2021 15:39:31 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> This change is mostly about tests. I say "mostly" because while increasing code coverage, which was the primary goal of this exercise, I uncovered a few non-critical bugs and fixed them in-place. The net effect of the change boils down to these code coverage statistics.
>> 
>> before
>> ------
>> 
>> %method	%block	%branch	%line
>> jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet	
>> 75%(12/16)
>> 87%(109/124)
>> 88%(99/112)
>> 85%(140/164)
>> 
>> #classes	%method	%block	%branch	%line
>> jdk.javadoc.internal.doclets.toolkit.taglets.snippet	
>> 70%(80/114)
>> 76%(310/407)
>> 65%(178/273)
>> 81%(413/508)
>> 
>> after
>> -----
>> 
>> %method	%block	%branch	%line
>> jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet	
>> 100%(17/17)
>> 95%(120/126)
>> 93%(103/110)
>> 97%(163/168)
>> 
>> %method	%block	%branch	%line
>> jdk.javadoc.internal.doclets.toolkit.taglets.snippet	
>> 83%(94/112)
>> 85%(348/405)
>> 73%(202/273)
>> 91%(463/505)
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address feedback
>   
>    - Hoist the tb (toolbox) field to SnippetTester
>    - Use use default ctors instead of private no-arg ctors for test classes
>    - Re-implement SnippetTester.checkOutputEither on top of JavadocTester.OutputChecker.checkAnyOf
>    - Follow convention for jtreg comment
>    - Improve method names with underscores

While I disagree with some of the code here, the code is not wrong, and so I'll approve the review to unblock the dependent work.

I hope we can clean up some of the issues later on.

Setting those concerns aside, overall, this is great work. Well done for such a thorough job for improving test coverage.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Attribute.java line 61:

> 59:  * deletion without notice.</b>
> 60:  */
> 61: public abstract class Attribute {

General question:   the commit message is:

> So we can finally use sealed classes where they were originally envisioned.

But, the code edits seem to be backing away from using sealed classes and interfaces.  What happened?

test/langtools/jdk/javadoc/doclet/testSnippetTag/SnippetTester.java line 96:

> 94:         var strings = Stream.concat(Stream.of(first), Stream.of(other))
> 95:                 .toArray(String[]::new);
> 96:         new OutputChecker(out).checkAnyOf(strings);

This works, and is a clever solution for now.  Maybe "one of these days" we remove the `checkOutputEither` method.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 98:

> 96:      */
> 97:     @Test
> 98:     public void testPositiveInlineTag_IdAndLangAttributes(Path base) throws IOException {

General comment, applying to all such situations ... I like the latest iteration on the naming scheme, with the embedded `_`.

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

Marked as reviewed by jjg (Reviewer).

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