Review request for type annotation reflection test

Charlie Wang charlie.wang at oracle.com
Wed Jun 19 01:04:12 PDT 2013 

Hi,
    Please review type annotation reflection test. Updated according to 
Alex's comments.
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/19 5:56, Alex Buckley wrote:
> Did you see my last review email?
>
> http://mail.openjdk.java.net/pipermail/type-annotations-dev/2013-June/001066.html 
>
>
> The webrev appears unchanged since the last time you sent its URL. Not 
> sure what to say about the text file showing test runs...
>
> Alex
>
> On 6/17/2013 7:51 PM, Charlie Wang wrote:
>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>>

More information about the type-annotations-dev mailing list