Typo in CatchExceptionTest.java: 66 private int droped; Should be “dropped”.
Otherwise seems ok... But more comments would be appreciated. It’s huge and really hard to understand. igor On Mar 28, 2014, at 9:04 AM, Igor Ignatyev <igor.ignat...@oracle.com> 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-...@openjdk.java.net >>>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >>>>>> >>>>>> _______________________________________________ >>>>>> mlvm-dev mailing list >>>>>> mlvm-...@openjdk.java.net >>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >>>>>> >>>>> _______________________________________________ >>>>> mlvm-dev mailing list >>>>> mlvm-...@openjdk.java.net >>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >>>> _______________________________________________ >>>> mlvm-dev mailing list >>>> mlvm-...@openjdk.java.net >>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >>> _______________________________________________ >>> mlvm-dev mailing list >>> mlvm-...@openjdk.java.net >>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev >> _______________________________________________ >> mlvm-dev mailing list >> mlvm-...@openjdk.java.net >> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev > _______________________________________________ > mlvm-dev mailing list > mlvm-...@openjdk.java.net > http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
