RFR: JDK-8224083: javadoc Reporter generates "warning" for Kind.NOTE

[Jonathan Gibbons]

Hi Vicente,

The fix for the source file looks good, but there are a few things to 
note with the test.

1. You check that "warning" does not appear, but you don't check that 
NullPointerException does not appear, which is the other bug you are fixing.

2. Tests that strings do not appear are typically very weak tests. If 
the test were to do nothing, or crash on startup, the designated text 
would not appear and the test would pass as a false positive.  While it 
is not wrong to test that text does not appear, it is generally better 
to augment it with a test to verify that some expected output does appear.

3. Using javac.util.Assert is a bit of overkill, since it means you have 
to expose it in the @modules clause.  I think it is better to just test 
the condition and throw an exception directly. Depending on and using 
javac internal classes is an anti-pattern we should avoid as much as 
possible.

4. You can simplify the test by combining the doclet into the test class 
itself, avoiding the use of a long multi-line string; this also has the 
side effect of compiling the doclet for you for free, so that you don't 
need to compile that string.You can see this technique used in a number 
of javadoc tests: try the following code to get a list of examples:

        grep -r 'implements Doclet' open/test/langtools

You should be able to merge the classes since your test code just 
"extends TestRunner" and a doclet "implements Doclet", which can both be 
accomplished in a single class.

If all that sounds too clever, I would move the demo doclet into a new 
source file, so that it can easily be modified (if needed) by a standard 
IDE, instead of having to be careful about editing Java source code 
embedded in a long string constant. Even if you move it into a new 
source file in a subdirectory of the test directory 
(test/langtools/jdk/javadoc/tool/reporter_generates_warnings/) you can 
still use jtreg to compile the file with an obvious use of @build.  
Given that the NPE was triggered by the doclet analyzing code in a named 
package, this is probably the better way to go ... to ensure that the 
pathway that used to generate NPE is also tested.

-- Jon


On 05/29/2019 03:27 PM, Vicente Romero wrote:
> Please review fix for [1] and [2] at [3]. These are two separate 
> issues for which the same test is provided so this one patch will fix 
> both. The first issue is related to 
> jdk.javadoc.internal.tool.Messager, producing warnings at method:
> print(Kind kind, Element e, String msg)
> even if it was instructed to produce notes. A case for Kind.NOTE was 
> added. The other issue was this same class throwing a NPE if it 
> couldn't find an element at method ::getDiagSource. In this case a 
> check for null is done and `programName` is returned as it is done 
> already for a similar case in that method,
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8224083
> [2] https://bugs.openjdk.java.net/browse/JDK-8224082
> [3] http://cr.openjdk.java.net/~vromero/8224083/webrev.00/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190529/8efaeb14/attachment.html>