Review request for type annotation reflection test

Alex Buckley alex.buckley at oracle.com
Thu Aug 8 10:05:52 PDT 2013 

- AnnotationTest.i/clsIdx: it wasn't the name of the variable I was 
objecting to. It was the fact that you're relying on a mutable public 
static variable at all.

- GenTestCode should take Class<? extends TestCaseGenerator>, so you 
don't need to cast to TestCaseGenerator in GenTestCode. (Same comment 
for AnnotationTest.compileCode).

- AnnotationTest.testInput is a Map<Object,String> but I'm sure you can 
do better than Object for keys.'

- ExecutableGetAnnotatedParameterTypesTest.testInput hides 
AnnotationTest.testInput, but swaps the order of the type arguments! 
Map<String,Object> is either wrong, or should not use Object.

Alex

On 8/8/2013 12:28 AM, Charlie Wang wrote:
> 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