RFR(S) : 8038186 : [TESTBUG] improvements of test j.l.i.MethodHandles

Sat Mar 29 08:27:15 UTC 2014

Igor,

Thanks for review.
> Should be “dropped”.
fixed

> But more comments would be appreciated. It’s huge and really hard to understand.
sure, I'll add them during future improvements.

Igor

On 03/29/2014 05:07 AM, Igor Veresov wrote:
> 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.ignatyev at 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.ignatyev at 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 at openjdk.java.net
>>>>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> mlvm-dev mailing list
>>>>>>> mlvm-dev at openjdk.java.net
>>>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>>>>>
>>>>>> _______________________________________________
>>>>>> mlvm-dev mailing list
>>>>>> mlvm-dev at openjdk.java.net
>>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>>> _______________________________________________
>>>>> mlvm-dev mailing list
>>>>> mlvm-dev at openjdk.java.net
>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>> _______________________________________________
>>>> mlvm-dev mailing list
>>>> mlvm-dev at openjdk.java.net
>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>> _______________________________________________
>>> mlvm-dev mailing list
>>> mlvm-dev at openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> _______________________________________________
>> mlvm-dev mailing list
>> mlvm-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>