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

[Vicente Romero]

Hi Jon,

I have published another iteration at [1], some comments below,

On 5/29/19 7:10 PM, Jonathan Gibbons wrote:
>
> 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.
>

the test fails if a NPE happens that's why I didn't see necessary to add 
a specific check for that

> 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.
>

I have modified the check and now I'm checking for a specific output.

> 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.
>
I removed the use of Assert
>
> 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

Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8224083/webrev.01/
>
>
> 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/20190530/90b141cb/attachment.html>