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

Thu Nov 18 18:32:51 UTC 2021

On Fri, 12 Nov 2021 16:28:09 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:
> 
>   Test one more corner case example

In general, great work.    Most of my comments are about style, especially unindented text blocks.

The one aspect that I think deserves attention is the code for the `@link` feature and the use of in-memory file objects and file managers. While not wrong, it seems overkill and unnecessary, since I think you could embed the reference tag and the markup tag in the same file, without going all the way to the separate run of javadoc.   Given that it's not wrong, and that there are dependent reviews blocked by this, I'll approve it if you want, but would like to understand if there are reasons to do it as you have.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 94:

> 92:     }
> 93: 
> 94:     private static final class BadSnippetException extends Exception {

General comment: I like the general use of `BadSnippetException`. Very nice.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 202:

> 200:                     fileObject = fileManager.getFileForInput(Location.SNIPPET_PATH, "", v);
> 201:                 }
> 202:             } catch (IOException | IllegalArgumentException e) { // TODO: test this when JDK-8276892 is integrated

FYI: The documented conditions of `IllegalArgumentException` do not arise in this context, although it's not wrong to catch the exception.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Replace.java line 79:

> 77:             replacements.add(new Replacement(start, end, s));
> 78:         }
> 79:         // there's no need to call matcher.appendTail(b)

(minor) maybe add a brief reason to the comment

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

> 88:     }
> 89: 
> 90:     // TODO: perhaps this method could be added to JavadocTester

There should be an equivalent feature there now

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 71:

> 69:  * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder builder.ClassBuilder
> 70:  * @run main TestSnippetMarkup
> 71:  */

The standard convention is to put the jtreg comment right after the legal header, before the imports.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 74:

> 72: public class TestSnippetMarkup extends SnippetTester {
> 73: 
> 74:     private final ToolBox tb = new ToolBox();

You might want to move this into `SnippetTester`

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 185:

> 183:                                 link(First) link(line)
> 184:                                   Second line
> 185:                                 """, "link\\((.+?)\\)", r -> link(true, "java.lang.Object#Object", r.group(1)))

clever

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 191:

> 189: 
> 190:     @Test
> 191:     public void testCornerCases(Path base) throws Exception {

nice corner cases

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 230:

> 228:      * next line.
> 229:      */
> 230: //    @Test

(minor) maybe add `// TODO`

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 309:

> 307:                ^""".formatted(badFile),
> 308: """
> 309: %s:1: error: snippet markup: tag refers to non-existent lines

The zero-indent on these lines makes them slightly less convenient to read in context.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 388:

> 386:         // generate documentation at low cost and do not interfere with the
> 387:         // calling test state; for that, do not create file trees, do not write
> 388:         // to std out/err, and generally try to keep everything in memory

I understand what you're doing here, but I'm surprised you need to go to the trouble of a separate run of javadoc to get the link output.   Can you not put the link tag and the link markup tag in the same source, and then extract the two `<a>` from the generated file?   Maybe put distinguishing characters around each instance, to make locating them easier.  For example:

 /**
  * First sentence.  TAG {@link Object} TAG.
  * {@snippet :
  *      MARKUP Object MARKUP // @link substring=Object target=Object
  * }
  */

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 395:

> 393:                 """.formatted(targetReference, content);
> 394: 
> 395:         JavaFileObject src = new JavaFileObject() {

Check out `SimpleJavaFileObject` for use as a super type.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 456:

> 454: 
> 455:         // FileManager has to be StandardJavaFileManager; JavaDoc is adamant about it
> 456:         class InMemoryFileManager extends ToolBox.MemoryFileManager

(Future?) this could be promoted to a top level class, or merged into the toolbox class.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java line 43:

> 41:  * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder builder.ClassBuilder
> 42:  * @run main TestSnippetPathOption
> 43:  */

before imports?

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java line 46:

> 44: public class TestSnippetPathOption extends SnippetTester {
> 45: 
> 46:     private final ToolBox tb = new ToolBox();

(minor) move to SnippetTester

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java line 131:

> 129:                 .map(Path::toAbsolutePath)
> 130:                 .map(Path::toString)
> 131:                 .collect(Collectors.joining(File.pathSeparator));

(minor) This sequence is showing up enough to warrant being a method somewhere.

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java line 168:

> 166:      */
> 167:     @Test
> 168:     public void testSearchPath(Path base) throws Exception {

Not wrong, but arguably somewhat superfluous. This is close to being a file manager test, not a snippet test.

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

> 1191: : error: snippet markup: missing attribute "region"
> 1192: // @start
> 1193:     ^""");

(style) why not indent the text?

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

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