On Fri, 19 Nov 2021 15:39:31 GMT, Pavel Rappo <pra...@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
