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

[Igor Ignatyev]

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