Review request for type annotation reflection test

[Alex Buckley]

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.

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

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