Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Wed Jun 19 16:21:17 PDT 2013 

- Please provide a text file for the generated code of each java/lang/* 
test.

- Two kinds of TestXYZGenerator is confusing. TestSourceGenerator should 
be called DeclarationGenerator. That's what it generates: declarations 
of classes, interfaces, fields, etc which are gathered up by the test 
case generators. Helper.SrcType should be called Helper.Declaration. The 
name of an enum type should describe what kind of things are represented 
by the enum constants, which here means declarations (of classes, 
interfaces, fields, etc). And you have a method called "genTestCode" in 
Helper as well as a method called "GenTestCode" in each test file (e.g. 
ClassGetAnnotatedInterfaceTest.java).

- It is extremely subtle that TYPE_ANNOTATION_1/2/3 represents both the 
declaration of an annotation type (to be part of a test case) _and_ an 
annotation (to be applied wherever #ANNO appears in the declaration 
under test). Please rename TYPE_ANNOTATION_1/2/3 to 
ANNOTATION_TYPE_1/2/3 (and, TYPE_ANNOTATION_CONTAINER_1/2/3 to 
CONTAINING_ANNOTATION_TYPE_1/2/3).

- There is too much duplication in the java/lang/* classes, and 
generally too much use of static fields. These classes should each be a 
subclass of an abstract class AnnotationTest, which declares test() and 
the bulk of the code in checkResult(). AnnotationTest should have a 
variable "annotationCombinations" (not "annoComb") to enumerate the 
annotations to be applied.

- Raw type Map in signature of Helper#genTestCode.

- ClassGetAnnotatedInterfaceTest -> missing an 's' after Interface. 
ClassGetAnnotatedSuperClassTest should be Superclass not SuperClass.

- @author should be a full name, not a first name.

Alex

On 6/19/2013 1:04 AM, Charlie Wang wrote:
> 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