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

[Jonathan Gibbons]

On Fri, 19 Nov 2021 10:02:26 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> 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.
>
> You are much more knowledgeable in FileManager than I am. My thinking was that while it's probably not an issue with the default file manager, I couldn't guarantee that IllegalArgumentException is not thrown if this code runs with a custom file manager.

I was just reading the documented conditions for this `JavaFileManager` method to throw `IllegalArgumentException`. That being said, one of the problems with ILA, and potentially other unchecked exception, is that they can occur from deep within the method that is called. 

FYI, javac has a way of wrapping user-provided components so that it can better handle unexpected exceptions from user-provided code as compared to unexpected exceptions in javac itself. If you're interested, see`javac` `ClientCodeWrapper`.   And, to be clear, I'm *not* suggesting that for javadoc at this time!

>> 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.
>
> I was surprised to hear that this is a convention, so much so that I wrote a script to survey the JDK tests in this regard. The script showed that while what you said is true for javadoc, it is not so true for other areas (even in langtools). That said, I can see the rationale for having this convention and will update the snippet tests accordingly.
> 
> Separately. If there's a value in mass-fixing the JDK tests so that they follow this convention, I can create a PR to do that. Here are some numbers provided by the script, to assess the situation. There are 32,920 `test/**/*.java` files. Of these, 19,190 have jtreg comments. Of these, 4,284 (or 22%) do not follow the convention one way or another. If pursued, that might result in an extremely tedious-to-review PR.

Maybe the convention is not as strong as I thought. It definitely does not warrant mass updates.

I'd be OK to review any updates needed for javadoc tests.

>> 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
>>   * }
>>   */
>
> What you suggested is how I had that initially. But then a test purist in me demanded a clear way of getting the link without interfering with the test logic.
> 
> As for text blocks: to me, indenting a text block is a complex, subjective and unsettled issue. I would postpone re-indenting them for now, primarily because of the churn it would otherwise introduce to the dependent PRs. 
> 
> If neither of these issues is a showstopper, I would really like to integrate this PR to unlock more urgent work on the snippets feature before RDP 1, which is on 2021/12/09 as per the Schedule at https://openjdk.java.net/projects/jdk/18/. Sometime after this PR is integrated, we can revisit this.

OK, but the test purist in me dislikes the significant amount of inline overhead to address this concern.

I'll pass on this for now, because I agree with unblocking the dependent PRs, but it seems like that is a negative side-effect of dependent PRs in general.  Once we get past this, at a minimum, I would suggest moving the in-memory file manager out of this class, and then potentially reconsider the decision that it is good practice to do the separate run.

>> 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.
>
> I can add a comment that this test is a part of an end-to-end demonstration of how --snippet-path should work. This is a black-box test that assumes nothing about the implementation of the feature.

OK, but I still think the test is somewhat unnecessary. But now you have it, don't delete it.

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

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