Review request for type annotation reflection test

Charlie Wang charlie.wang at oracle.com
Tue Aug 6 20:49:28 PDT 2013 

Alex,
   Thank you for the careful checking. You can add/remove/change 
annotationCombinations in AnnotationTest, as long as the strings are 
legal annotations. And as to the other comments, I've updated my code. 
Please take a look again.

http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/webrev/



- Charlie

On 2013/8/3 7:55, Alex Buckley wrote:
> 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