Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Mon Jun 3 14:52:23 PDT 2013 

Hi Charlie,

First, I think there is some clever stuff going on here, but it's hard 
to figure out what it is. It's not enough to sprinkle comments like "// 
input should be like ["@A() @B()", "@C() @D()"...]" in SrcType. I would 
like an explanation of what a test case like ClsGetAnnotatedInterTest is 
trying to do. The TestCase enum type looks like a good place to start. 
Then, I would like to know how Helper.SrcType supports that with 
search-and-replace on keys like #ANNO. To be clear, I'm not looking for 
three bullet points; I'm looking for paragraphs that can be added to 
your code's javadoc and be useful to someone in ten years time.

Second, in the interest of increasing the abstraction of Helper.SrcType, 
you should introduce an interface with a getSrc method and have it be 
implemented by the SrcType enum type. You should also remove duplicated 
code throughout SrcType's enum constant bodies (e.g. the code for "// 
get @anno for [#ANNO1] ...").

Third, please don't shorten terms which are part of the Java language. 
METHODHEAD should be METHOD_HEADER. METHODPARAM should be 
METHOD_PARAMETER. Inter should be Interface. TYPEANNO1 should be 
TYPE_ANNOTATION_1. PKGANNOTATION1 should be PACKAGE_ANNOTATION_1. And so 
on. Rigor really matters when dealing with the Java language.

Alex

On 6/2/2013 11:39 PM, Charlie Wang wrote:
> Hi,
>    Please review attached 2 files, which I've changed according to
> comments. If OK, I will update the rest of the tests and publish for
> review.
>
> Regards,
> Charlie
> On 2013/5/22 9:55, Joseph Darcy wrote:
>> Hello,
>>
>> On 5/21/2013 6:37 PM, Alex Buckley wrote:
>>> I see you have extracted the validation logic and common annotation
>>> types into TestUtil, so that any individual test will primarily be a
>>> data provider + test cases. However:
>>>
>>> 1. Jon is our resident TestNG expert and he says that data providers
>>> are intended to enumerate input values for tests; other code
>>> determines which values should pass the tests and which values should
>>> fail. But your data provider encodes the types and element values of
>>> annotations which are expected to appear at various locations - this
>>> is output data, not input data.
>>>
>>> 2. There is basically no abstraction over the expected annotations in
>>> the data provider. It's just a bunch of Object[][] values.
>>> Consequently, TestUtil is very brittle. There is weird indexing of
>>> arrays - [i * 2 + 1] and [j / 2] all over the place - and excessive
>>> knowledge of specific test cases:
>>>
>>>   catch (NoSuchMethodException e) {
>>>     //expected exception for TypeAnno3
>>>   }
>>>
>>> 3. I'm suspicious of static methods in TestUtil. They are stateless
>>> at present, but if they ever share state then you'll be in trouble,
>>> especially with the instance methods in GetAnnotated*Test.
>>>
>>> Charlie, there are two techniques you should adopt for tests on the
>>> core reflection API:
>>>
>>> - First, encode expected values of type annotations more directly, by
>>> annotating the AnnotationTypeTestXX declarations themselves.
>>
>> Note that this technique is used in various other regression tests in
>> the JDK repository. For some recently-written small examples see
>>
>>     test/java/lang/reflect/Parameter/WithoutParameters.java
>>     test/java/lang/reflect/Method/DefaultMethodModeling.java.html
>>
>>>
>>> - Second, embrace combinatorial testing rather than declaring a bunch
>>> of ad-hoc AnnotationTypeTestXX types with type annotations in various
>>> locations.
>>>
>>> The core reflection tests for repeating annotations use both these
>>> techniques. Please look at item 2 of
>>> http://wiki.se.oracle.com/display/JPG/Repeating+Annotation+Test+Plan#RepeatingAnnotationTestPlan-1.Featurecases
>>> - apologies for non-Oracle readers. Please talk with Steve and
>>> Matherey to get background. To be clear, I will not sign off the type
>>> annotations reflection tests in their present form.
>>
>> I concur that using combinatorial tests is appropriate for this
>> domain. The basic approach is to programatically generate both the
>> expected result and the code to test and verify that the computed
>> answer is consistent with the expected one. The regression tests
>> currently in JDK 8 have code that allows source code to be generated
>> at runtime, loading in by a class loader, and then run.
>>
>> HTH,
>>
>> -Joe
>>
>>>
>>> Alex
>>>
>>> On 5/21/2013 2:29 PM, Charlie Wang wrote:
>>>> OK, thanks.
>>>> I will create a TestUtil.java (as attached file). Extract the common
>>>> part and put them in it. And Add comments on data provider.
>>>> I'm on a very tight schedule, so this time I just send out two of the
>>>> updated files (attached files) for review so I can get a quick
>>>> response.
>>>> If it is OK, I will use it as a template on other tests.
>>>> Please take a look and send me your comments as soon as possible.
>>>>
>>>>
>>>> Thanks,
>>>> Charlie
>>>>
>>>>
>>>> On 2013/5/22 2:24, Alex Buckley wrote:
>>>>> Please keep one source file per tested API method, because it is easy
>>>>> to navigate. The problem is the body of the test() method in each
>>>>> source file. It should call utility methods to inspect annotations,
>>>>> rather that repeating more or less the same 'for' loops as every other
>>>>> test() method.
>>>>>
>>>>> As for data providers, I believe they are a TestNG thing. Please add
>>>>> comments to each source file to describe what the data provider
>>>>> represents.
>>>>>
>>>>> Finally, when you make a "little update", you should give a short
>>>>> description of what has changed. If you don't, someone who looked at
>>>>> the previous version has no idea what changed and will have to look at
>>>>> everything again.
>>>>>
>>>>> Alex
>>>>>
>>>>> On 5/21/2013 3:10 AM, Charlie Wang wrote:
>>>>>> Yes, that make sense. What do you think if I combine the tests? It
>>>>>> would
>>>>>> then have 1 unified data provider and test method, thus solve the 2
>>>>>> problems.
>>>>>>
>>>>>> Regards,
>>>>>> Charlie
>>>>>> On 2013/5/21 14:56, Werner Dietl wrote:
>>>>>>> I agree with Alex's first two comments. The code duplication for the
>>>>>>> test logic and all the different annotation types seems hard to
>>>>>>> maintain and update.
>>>>>>>
>>>>>>> cu, WMD.
>>>>>>>
>>>>>>> On Mon, May 20, 2013 at 7:56 PM, Charlie Wang
>>>>>>> <charlie.wang at oracle.com> wrote:
>>>>>>>> Hi,
>>>>>>>>    Here's the latest one with a little update:
>>>>>>>> http://cr.openjdk.java.net/~ssides/8013497/webrev.03/
>>>>>>>>    Please provide comments as soon as possible now that due date is
>>>>>>>> approaching.
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Charlie
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2013/5/19 14:10, Charlie Wang wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>    Here's test for core reflection support of type-annotations
>>>>>>>> JDK-8013497.
>>>>>>>> Please take a look.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ssides/8013497/webrev.02/
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Charlie
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

More information about the type-annotations-dev mailing list