Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Fri Aug 2 16:55:33 PDT 2013 

Hi Charlie,

This is much more understandable with the *.txt files, but still seems 
brittle. What would happen if you added a new String to 
annotationCombinations in AnnotationTest, or changed an existing String? 
Helper.Declaration is very precise about how many annotations are used.

Also:

- Some source files like TestCaseGenerator.java have text before the 
license comment, which I'm pretty sure is a no-no.

- The introductory comment in each *Test class refers to 
annotationCombinations, but it would help to say 
AnnotationTest.annotationCombinations.

- Sometimes the TestCase enum is private for no reason.

- Many checkResult methods have code which could be shared.

- The GenTestCode methods are strange. Each has to step through the 
TestCase values very carefully, and it's weird to see an enhanced-for 
loop with a counter variable (i). You should pass any values needed by 
TestCase.genTestCase to the constructor of the enum constant.

Alex

On 7/31/2013 8:03 PM, Charlie Wang wrote:
> Hi Alex,
>    I've put a *.txt file for each of the tests.
>    Also there has been some update on the code:
> 1. final fiels declaration,
> 2. extraction of the field "lhm" into TestCaseGenerator,
> 3. removal of some redundant code,
> 4. replace StringBuffer with StringBuilder because synchronization is
> implemented outside,
> 5. removal of some unused imports,
> 6. extraction of common parts into methods.
>
> Here's the test:
> http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/webrev/
> Sorry for the long gap since your last review comments. Tried to make
> some big improvements on the code.
>
>
>
> Thanks,
> Charlie
>
> On 2013/6/20 7:21, Alex Buckley wrote:
>> - 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