Looks good to me.
Best regards,
Vladimir Ivanov
On 3/28/14 8:04 PM, Igor Ignatyev wrote:
Vladimir,
as we'd arranged, I moved ThrowMode into TestCase and give my word that
I'll
remove TestCase.assertEQ and enhance Asserts.assertEquals for arrays
add the test cases from TestCatchException to 'MANDATORY_TEST_CASES'
in CatchExceptionTest and remove TestCatchException
in JDK9 TF.
updated webrev: http://cr.openjdk.java.net/~iignatyev/8038186/webrev.03/
Thanks,
Igor
On 03/28/2014 03:31 PM, Igor Ignatyev wrote:
Vladimir, thanks for review, please my comments inline.
On 03/28/2014 01:29 PM, Vladimir Ivanov wrote:
Igor, thanks for improving the tests on method handles!
After an offline conversation w/ VladimirI, I've done several changes:
- placed all classes into one file (but I don't like it)
You can convert TestCase & TestFactory to inner classes and move
ThrowMode into TestCase since it's usage is confined to TestCase.
I don't like using of inner classes, from my point of view, it makes
code less readable. But if you insists, I can do it.
- changed package name
- extracted creating tests code from test execution code
These changes look good.
Other comments:
- why don't you remove TestCase.assertEQ and enhance
Asserts.assertEquals for arrays?
I'm not sure that this doesn't break the other tests which use
'Asserts.assertEquals', and since I want to push this patch to 8u20, I
consider it's too risky to change now.
I'll do it in my next improvements of MH tests in jdk9 TF.
- regarding CatchExceptionTest name: there's already
TestCatchException test. Have you considered merging them into one test
or removing TestCatchException, if your test already covers that cases?
I didn't remove TestCatchException, since I'm not pretty sure that all
cases are covered and don't have enough time to check it, also this
test's a regression test which cover some particular test cases.
I plan to add the test cases from TestCatchException to
'MANDATORY_TEST_CASES' in CatchExceptionTest and remove
TestCatchException in jdk9 TF.
Best regards,
Vladimir Ivanov
PS: John mentioned recently that there's a convention for
java/lang/invoke tests to be in JUnit format. I'm not sure it'll improve
the code in any way (maybe TestNG will?). I hope John will express his
opinion on this, if it's important.
I hope the code's still quite readable and understandable.
Also I've added array return type.
http://cr.openjdk.java.net/~iignatyev/8038186/webrev.02/
Thanks,
Igor
On 03/27/2014 12:46 AM, Igor Ignatyev wrote:
Hi Chris,
This is very nice. Are there plans to do this for the other existing
tests as well?
sure, but we didn't have enough time to do it in 8u20 timeframe. So
I'd
like to push these changes 1st, since we need this to cover 'Speedup
catch exceptions'. Other tests will be rewrite later. I hope we'll
have
time in jdk 9 to refactor all tests in MethodHandles.
Thanks,
Igor
On 03/27/2014 12:38 AM, Christian Thalinger wrote:
On Mar 24, 2014, at 1:33 PM, Igor Ignatyev <igor.ignat...@oracle.com>
wrote:
Hi all,
Please review the patch:
Problems:
- MethodHandlesTest::testCatchException() doesn't provide enough
testing of j.l.i.MethodHandles::catchException().
- MethodHandlesTest contains more than 3k lines, an auxiliary code
together w/ a test code, many methods aren't connected w/ each
other,
etc. All this leads to that it is very very hard to understand,
maintain.
Yes, it is.
Fix:
- the auxiliary code was moved to testlibray
- the test code was moved to a separate subpackage
This is very nice. Are there plans to do this for the other existing
tests as well?
- random signature is used for target and handler
- added scenarios w/ different return types
webrev: http://cr.openjdk.java.net/~iignatyev/8038186/webrev.01/
jbs: https://bugs.openjdk.java.net/browse/JDK-8038186
--
Igor
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
