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

Sat Mar 29 01:07:31 UTC 2014

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