Review request for type annotation reflection test

Mon Jun 17 19:51:25 PDT 2013

Hi,
   Please review type annotation reflection test.
http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/webrev/

   Here's test result:
http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/1.txt

Regards,
Charlie
On 2013/6/13 10:41, Charlie Wang wrote:
> Hi,
>   Updated all the tests according to comments. Here's webrev. Please 
> review.
> http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/webrev/
>
>
>
> Regards,
> Charlie
>
>
> On 2013/6/4 5:52, Alex Buckley wrote:
>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>