Review request for type annotation reflection test

Charlie Wang charlie.wang at oracle.com
Thu Aug 8 00:28:01 PDT 2013 

Alex,
   Thanks for the quick reply. The index number i is just a unique 
suffix appended on the class, which is temporarily created for test. I 
don't think it is a problem. But for easier understanding, I've change 
its name to clsIdx. And as to the other comments, I've updated my code 
accordingly. Please take a look again.
http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation/webrev/ 
<http://cr.openjdk.java.net/%7Epzhang/Charlie/TypeAnnotation/webrev/>




- Charlie


On 2013/8/8 1:01, Alex Buckley wrote:
> - AnnotationTest line 49 is:
>   // index number for test class names
>   public static int i = 0;
> That just doesn't smell right.,
>
> - Still some blank lines before licenses.
>
> - GenTestCode is now much much simpler! Could it be pulled up into 
> AnnotationTest, then overridden in the various *Test classes (calling 
> super() then doing additional work) where necessary?
>
> - checkResult methods all have similar debugPrint calls which should 
> be extracted into a Helper method. ("falty" is spelled wrong.)
>
> Alex
>
> On 8/6/2013 8:49 PM, Charlie Wang wrote:
>> Alex,
>>    Thank you for the careful checking. You can add/remove/change
>> annotationCombinations in AnnotationTest, as long as the string 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