Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Wed Aug 7 10:01:14 PDT 2013 

- 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