Review request for type annotation reflection test

Alex Buckley alex.buckley at
Thu Jun 13 12:29:41 PDT 2013

Thanks Charlie. I can see the combinatorial approach emerging, but the 
abstraction level still needs to be raised to make the code 
approachable. This can be achieved with more interfaces and less 
duplication, as suggested below.

- Some of the files have their last line missing.

- Please check the jtreg guidelines for how to write @test, @bug, and 

- PkgGetDeclaredAnnotations should end in Test. Also, don't abbreviate - 
there is no value saying Cls rather than Class, Pkg rather than Package, 
and Var rather than Variable, it just trips over the reader.

- Here's a bunch of stuff about organization. First, please separate the 
infrastructure code into a different package than the Test classes. 
Second, the Test classes themselves should be in named packages, not in 
unnamed packages. Third, the directory test/java/lang/typeannoreflection 
is really painful because there is no package 
java.lang.typeannoreflection. Ideally you would have 
test/type-annotations/java/lang/Class for the test classes which 
exercise java.lang.Class, and 
test/type-annotations/java/lang/reflect/Executable for the test classes 
which exercise j.l.r.Executable methods, and so on.

- annoComb seems to be identical in every Test class. Why not centralize 
it? Methods like compareAnnoWithDiffSeq and debugPrint are also 
duplicated in various classes.

- The TestCase enum constants are all intended to implement a 
getTestCase method, so please declare an interface for TestCase to 

- Style: Taking this as an example: "static LinkedHashMap lhm = new 
LinkedHashMap();" ... 1) It is not acceptable to use raw types in Java 
code written in 2013, and 2) Declare variables using interfaces 
(Map<...>) not classes (LinkedHashMap<..>) wherever possible.


On 6/12/2013 7:41 PM, Charlie Wang wrote:
> Hi,
>    Updated all the tests according to comments. Here's webrev. Please
> review.
> 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.
>> METHOD_PARAMETER. Inter should be Interface. TYPEANNO1 should be
>> 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/
>>>> test/java/lang/reflect/Method/
>>>>> - 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
>>>>> - 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 (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
>>>>>>>>> < at> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>    Here's the latest one with a little update:
>>>>>>>>>>    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.
>>>>>>>>>> Regards,
>>>>>>>>>> Charlie

More information about the type-annotations-dev mailing list