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

[Vicente Romero]

thanks,
Vicente

On 5/31/19 6:01 PM, Jonathan Gibbons wrote:
>
> Nice;
>
> I like all the specific positive tests.
>
> -- Jon
>
>
>
> On 05/30/2019 09:48 AM, Vicente Romero wrote:
>> 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/20190531/c1772397/attachment-0001.html>