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

[Jonathan Gibbons]

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/398d7eaf/attachment.html>