Code Review Request: More tests for 7184826: (reflect) Add support for Project Lambda concepts in core reflection

[Dan Smith]

Hi.

Per a request from Joel, I've taken a look at DefaultStaticTestData.  I don't really have the full context here, but I'm assuming that the annotations get translated into tests that guarantee 1) the result of Class.getMethods is exactly (no more -- excepting Object methods -- and no less) those methods named by MethodDesc annotations; and 2) the result of Class.getDeclaredMethods is exactly (no more, no less) those methods that are marked "declared=YES".

The expected results seem accurate.  I would personally focus testing more on different inheritance shapes and less on different combinations of (unrelated) method declarations or presence/absence type variables (!?), but it's a valid test in any case.

There ought to be some testing for scenarios that javac won't generate, like conflicting default method declarations.

It's not clear to me why we're generating classes with lambda methods (TestIF16) and then not testing that reflection sees those methods.  Presumably they get filtered out by the testing logic. But, if so... what's the point of testing?  Really, I don't think lambdas belong here at all -- it's a completely orthogonal feature.

(Note: I'm not a Reviewer either, but don't mind providing feedback, especially on spec-related questions.)

—Dan

On Jul 22, 2013, at 7:12 AM, Amy Lu <amy.lu at oracle.com> wrote:

> Thank you Joel for all the valuable feedback.
> 
> Test have been refactored and here comes the updated version:
> https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.01/index.html
> 
> Thanks,
> Amy
> 
> 
> On 7/10/13 10:17 PM, Joel Borggren-Franck wrote:
>> Hi Amy, Tristan,
>> 
>> I'm not a Reviewer kind of reviewer, but I've started to look at the
>> code and can sponsor this.
>> 
>> Some comments on test/java/lang/reflect/Method/invoke/DefaultStaticTest.java:
>> 
>> As there are a lot of non-public top-level classes perhaps this test
>> should be in it own directory.
>> 
>> It is very hard to read the data table:
>> 
>> 292             {interface1.class, 1, 1, new Object1(), new Object[]{},
>> 293                 new Object[]{}, "interface1", null},
>> 
>> I believe you should move the methodsNum constant and the declMethods
>> num constant to an annotation on the interface/class in question. For
>> example:
>> 
>> @MyTestData(numMethods=1, declMethods=1)
>> 41 interface interface1 {
>> 42     @DefaultMethod(isDefault = true)
>> 43     @StaticMethod(isStatic = false)
>> 44     @ReturnValue(value = "interface1.defaultMethod")
>> 45     default String defaultMethod(){ return "interface1.defaultMethod"; };
>> 46 }
>> 
>> That way it is much easier to inspect that the constants are right.
>> 
>> The same can probably be done with the return values encoded.
>> 
>> Instead of all these "new Objects[] {}" can't you create a field,
>> 
>> Object [] EMPTY_PARAMETERS = new Object() {}
>> 
>> and reuse it?
>> 
>> That way it will be much easier to verify that the encoded test data is
>> correct.
>> 
>> I'll comment on the other tests shortly.
>> 
>> cheers
>> /Joel
>> 
>> On 2013-06-13, Amy Lu wrote:
>>> This has been pending on review for long ... Please help to review.
>>> I also need a sponsor for this.
>>> 
>>> Thank you very much.
>>> 
>>> /Amy
>>> 
>>> On 5/23/13 10:48 PM, Amy Lu wrote:
>>>> Please help to review:
>>>> 
>>>> More tests for 7184826: (reflect) Add support for Project Lambda
>>>> concepts in core reflection
>>>> 
>>>> This includes:
>>>> 1. improvement for existing tests with more situations
>>>> 2. Test for invoking interface default/static method by j.l.r.Method
>>>> 3. Test for invoking interface default/static method by MethodHandle
>>>> 
>>>> https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.00/index.html
>>>> 
>>>> 
>>>> Thanks,
>>>> Amy
>>> 
>