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

[Vladimir Ivanov]

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