[9] RFR (S) 6191224: (reflect) Misleading detail string in IllegalArgumentException thrown by Array.get<Type>

Chris Plummer chris.plummer at oracle.com
Thu Oct 23 14:14:17 UTC 2014 

Thanks everyone!

Chris

On 10/23/14 4:46 AM, Lois Foltan wrote:
> Looks good, thanks for making the changes.  I don't need to see 
> another review either for fixing the one failTest call that David 
> pointed out.
> Lois
>
> On 10/22/2014 10:29 PM, Chris Plummer wrote:
>> Hi Lois and Christian,
>>
>> I had implemented David's suggestion to include the exception 
>> message, but he said he didn't need to review it again, so I didn't 
>> update the webrev. However, since you both have further suggestions, 
>> here's a new webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/6191224/webrev.04/
>>
>> 1. Include exception message in error output
>> 2. Change exception message to "Argument is not an array of primitive 
>> type".
>> 3. avoid using exception_caught
>>
>> thanks,
>>
>> Chris
>>
>> On 10/21/14 1:08 PM, Christian Tornqvist wrote:
>>> Hi Chris,
>>>
>>> Sorry for the late review, catching up on my emails.
>>>
>>> I agree with Lois here, please make sure the original exception 
>>> message is
>>> included in the exception being thrown when we fail.
>>>
>>> Minor style issue:
>>>
>>> You could get rid of the use of exception_caught by moving the 
>>> failTest()
>>> calls for no exception being thrown (Line 56 & 77) into the try 
>>> blocks. This
>>> should work since you're throwing an error in failTest() and only 
>>> catching
>>> exceptions in your catch blocks.
>>>
>>> Thanks,
>>> Christian
>>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev
>>> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Lois
>>> Foltan
>>> Sent: Tuesday, October 21, 2014 7:05 AM
>>> To: Chris Plummer
>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: [9] RFR (S) 6191224: (reflect) Misleading detail string in
>>> IllegalArgumentException thrown by Array.get<Type>
>>>
>>>
>>> On 10/21/2014 1:02 AM, Chris Plummer wrote:
>>>> I'm still looking for one more reviewer for this change.
>>> Hi Chris,
>>>
>>> I think this looks good.  Two minor comments:
>>>
>>> - src/share/vm/prims/jvm.cpp
>>> Following the wording choices used in the JLS 8 spec, there doesn't 
>>> seem to
>>> be use of the phrase "primitive type array".  I would prefer the error
>>> message to state "Argument is not an array of primitive type".
>>>
>>> - test/runtime/reflect/ArrayGetIntException.java
>>> I didn't see the addition of the e.getMessage() when reporting the 
>>> incorrect
>>> message.  I assume you added Christian's suggestion.
>>>
>>> Thanks,
>>> Lois
>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 10/13/14 7:15 PM, Chris Plummer wrote:
>>>>> On 10/13/14 6:29 PM, David Holmes wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> Two minor things:
>>>>>>
>>>>>>   27  * @summary D(reflect) Misleading detail string
>>>>>>
>>>>>> Stray D?
>>>>> Yeah. Something went wrong when I pasted in the bug synopsis. I'll
>>>>> remove it.
>>>>>> When reporting an incorrect message you should include the actual
>>>>>> message that was incorrect eg
>>>>>>
>>>>>> failTest("Test #3 FAILS - incorrect message: " + e.getMessage());
>>>>> Ok.
>>>>>> No need for re-review.
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 14/10/2014 8:26 AM, Chris Plummer wrote:
>>>>>>> Hi David and Christian,
>>>>>>>
>>>>>>> New Webrev:
>>>>>>> http://cr.openjdk.java.net/~cjplummer/6191224/webrev.03/
>>>>>>>
>>>>>>> changes:
>>>>>>> -Renamed to reflect/ArrayGetIntException.java -othervm argument
>>>>>>> removed -throws Error on failure rather than exit(1) -Indentation
>>>>>>> set at 4 instead of 2 -Open brace at end of line instead of at
>>>>>>> start of new line.
>>>>>>>
>>>>>>> When the test fails, it now outputs:
>>>>>>>
>>>>>>> STDOUT:
>>>>>>> 2147483647
>>>>>>> Test #1 PASSES
>>>>>>> java.lang.IllegalArgumentException: Argument is not an array Test
>>>>>>> #2 FAILS - incorrect message
>>>>>>> STDERR:
>>>>>>> java.lang.Error: Test #2 FAILS - incorrect message
>>>>>>>       at 
>>>>>>> ArrayGetIntException.failTest(ArrayGetIntException.java:83)
>>>>>>>       at ArrayGetIntException.main(ArrayGetIntException.java:56)
>>>>>>>       at sun.reflect.NativeMethodAccessorImpl.invoke0(Native 
>>>>>>> Method)
>>>>>>>       at
>>>>>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImp
>>>>>>> l.java:62)
>>>>>>>
>>>>>>>
>>>>>>>       at
>>>>>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAcc
>>>>>>> essorImpl.java:43)
>>>>>>>
>>>>>>>
>>>>>>>       at java.lang.reflect.Method.invoke(Method.java:483)
>>>>>>>       at
>>>>>>> com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.jav
>>>>>>> a:94)
>>>>>>>
>>>>>>>       at java.lang.Thread.run(Thread.java:745)
>>>>>>>
>>>>>>> JavaTest Message: Test threw exception: java.lang.Error: Test #2
>>>>>>> FAILS - incorrect message JavaTest Message: shutting down test
>>>>>>>
>>>>>>> STATUS:Failed.`main' threw exception: java.lang.Error: Test #2
>>>>>>> FAILS - incorrect message
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 10/8/14 10:53 PM, Chris Plummer wrote:
>>>>>>>> Hi Christian,
>>>>>>>>
>>>>>>>> [If this went out twice, ignore the other copy.]
>>>>>>>>
>>>>>>>> On 10/8/14 7:40 PM, Christian Tornqvist wrote:
>>>>>>>>> Hi Chris,
>>>>>>>>>
>>>>>>>>> First of all, thanks for writing a regression test :)
>>>>>>>>>
>>>>>>>>> The naming of the test should follow the Hotspot test naming
>>>>>>>>> guidelines
>>>>>>>>>
>>> (https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests). 
>>>
>>>>>>>>>
>>>>>>>> I'll wait for your response to David.
>>>>>>>>>> 28  * @run main/othervm Test6191224
>>>>>>>>> Is there a reason you need to run it with /othervm?
>>>>>>>> Copy and paste from another test case. Probably no need for it.
>>>>>>>>> When the test fails you should throw an exception instead of
>>>>>>>>> calling System.exit(), jtreg doesn't like System.exit
>>>>>>>> Ok. This was also copy and paste from the test case I used as an
>>>>>>>> example. It seems Error is common for test failures, so I'll use
>>>>>>>> that.
>>>>>>>>> (http://openjdk.java.net/jtreg/faq.html#question2.6)
>>>>>>>> "% bringover -p <workspace> test". A bit dated perhaps. ;)
>>>>>>>>
>>>>>>>> I'll get a new webrev out once I've cleaned it up.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>> Thanks,
>>>>>>>>> Christian
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: hotspot-runtime-dev
>>>>>>>>> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf
>>>>>>>>> Of Chris Plummer
>>>>>>>>> Sent: Wednesday, October 8, 2014 8:08 PM
>>>>>>>>> To:hotspot-runtime-dev at openjdk.java.net
>>>>>>>>> Subject: [9] RFR (S) 6191224: (reflect) Misleading detail string
>>>>>>>>> in IllegalArgumentException thrown by Array.get<Type>
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review this cleanup of a misleading exception message.
>>>>>>>>>
>>>>>>>>> Summary: The test case shows that an exception is thrown with the
>>>>>>>>> message "Argument is not an array", when in fact the argument is
>>>>>>>>> an array, but a primitive type array is actually what was
>>>>>>>>> expected. Fixed by differentiating between failing because an
>>>>>>>>> array was expected and failing because a primitive type array was
>>>>>>>>> expected.
>>>>>>>>>
>>>>>>>>> Webrev:http://cr.openjdk.java.net/~cjplummer/6191224/webrev.00/
>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-6191224
>>>>>>>>>
>>>>>>>>> Tested with:
>>>>>>>>>        jprt "-testset hotspot"
>>>>>>>>>        JTReg with hotspot tests and some jdk tests
>>>>>>>>>        JCK vm, lang, api
>>>>>>>>>        vm.quick.testlist
>>>>>>>>>        runthese
>>>>>>>>>
>>>>>>>>> Output for the regression test when passing is:
>>>>>>>>>
>>>>>>>>> 2147483647
>>>>>>>>> Test #1 PASSES
>>>>>>>>> java.lang.IllegalArgumentException: Argument is not a primitive
>>>>>>>>> type array Test #2 PASSES
>>>>>>>>> java.lang.IllegalArgumentException: Argument is not an array Test
>>>>>>>>> #3 PASSES
>>>>>>>>>
>>>>>>>>> And when it fails:
>>>>>>>>>
>>>>>>>>> 2147483647
>>>>>>>>> Test #1 PASSES
>>>>>>>>> java.lang.IllegalArgumentException: Argument is not an array Test
>>>>>>>>> #2 FAILS - incorrect message
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list