[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 02:29:27 UTC 2014 

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