Review request for type annotation reflection test
Charlie Wang
charlie.wang at oracle.com
Wed Jul 31 20:03:13 PDT 2013
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