On 10/11/16 19:50, David Holmes wrote:
Hi Serguei,
Thanks for looking at this.
On 12/10/2016 12:37 PM, serguei.spit...@oracle.com wrote:
Hi David,
It looks good, thank you for test improvements.
One minor comment.
http://cr.openjdk.java.net/~dholmes/8165827/webrev/test/com/sun/jdi/InterfaceMethodsTest.java.frames.html
511 private Method testLookup(ReferenceType targetClass, String
methodName, String methodSig,
512 boolean declaredOnly, Class<?> expectedException) {
513
514 System.err.println("Looking up " + targetClass.name() + "." +
methodName + methodSig);
515 try {
516 Method m = declaredOnly ?
517 lookupDeclaredMethod(targetClass, methodName, methodSig) :
518 lookupMethod(targetClass, methodName, methodSig);
519
520 if (expectedException == null) {
521 System.err.println("--- PASSED");
522 return m;
523 }
524 }
525 catch (Throwable t) {
526 if (t.getClass() != expectedException) {
527 System.err.println("--- FAILED");
528 failure("FAILED: got exception " + t + " but expected exception "
529 + expectedException.getSimpleName());
530 return null;
531 }
532 else {
533 System.err.println("--- PASSED");
534 return null;
535 }
536 }
537 System.err.println("--- FAILED");
538 failure("FAILED: lookup succeeded but expected exception "
539 + expectedException.getSimpleName());
540 return null;
541 }
I'd be better to keep the fragments 520-523 and 537-540 together as
they are logically bound.
Perhaps, it is better to move the 520-523 to move before the L537.
You're right - but I prefer to move the code from L537 into an else
for the if at L520. Webrev updated in place.
It's up to you.
There are more cases to use the testLookup() in this test but it is
probably for future improvements.
Yes - see the bugs I linked as [1] and [2].
Right. Perhaps, the it is a part of the JDK-8166453.
There are even more bugs related to static interface method handling
that impact this test. Bit of a can-of-worms.
BTW, would it make sense to consider one more test case ?
private void testImplementationClass(ReferenceType targetClass,
ObjectReference thisObject) {
. . .
testInvokeNeg(targetClass,thisObject, "privateMethodB", "()I",
vm().mirrorOf(RESULT_B),
"private interface methods are not inheritable");
Thanks, Serguei
Thanks, David -----
Thanks, Serguei On 10/10/16 18:55, David Holmes wrote:
Turns out the only place changes were needed were in JDI. Bug:
https://bugs.openjdk.java.net/browse/JDK-8165827 webrev:
http://cr.openjdk.java.net/~dholmes/8165827/webrev/ The spec change
in ObjectReference is very simple and there is a CCC request in
progress to ratify that change. The implementation change in
ObjectReferenceImpl mirrors the updated spec and use the same format
as already present in the class version of the check method. The
test is a little more complex. This is obviously an extension to
what is already tested in InterfaceMethodsTest. However IMT has a
number of problem with the way it is currently written [1] -
specifically it doesn't properly separate method lookup from method
invocation. So I've added the capability to separate lookup and
invocation for use with the private interface methods - I have not
tried to address shortcomings of the existing tests. Though I did
fix the return value checking logic! And did some clarifying
comments and renaming in a couple of place. Still on the test I
can't add the negative tests I would like to add because they
actually pass due to a different long standing bug in JDI - [2]. So
the actual private interface method testing is very simple: can I
get the Method from the InterfaceType for the interface declaring
the method? Can I then invoke that method on an instance of a class
that implements the interface. Thanks, David [1]
https://bugs.openjdk.java.net/browse/JDK-8166453 [2]
https://bugs.openjdk.java.net/browse/JDK-8167416
