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

[Amy Lu]

Thank you Dan !

Please see my comments inline...

On 7/24/13 5:12 AM, Dan Smith wrote:
> 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.
Testing on "javac" is out of this scope, it's covered by langtools 
tests, say test/tools/javac/defaultMethods/
>
>
> 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.
This testing focus on the locating and invoking the default and static 
methods by core reflection API.

Test case with lambda expression in default method is trying to test 
that the "default" method won't be broken by the lambda expression (it 
should be found and invokable as normal). Testing on the Lambda 
expression itself is covered by other tests.

Thanks,
Amy
>
>
> (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
>>>>
>>
>