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...@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
