Review request for type annotation reflection test

Charlie Wang charlie.wang at oracle.com
Wed Jun 12 19:41:59 PDT 2013 

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

More information about the type-annotations-dev mailing list