Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Tue Jun 18 14:56:42 PDT 2013 

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