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

David Holmes david.holmes at oracle.com
Thu Oct 23 02:59:00 UTC 2014 

Hi Chris,

On 23/10/2014 12: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

You missed:

   42             failTest("Test #1 FAILS - legal access denied");

Otherwise looks fine. Again no need to see an update if the e.getMessage 
to line 42 is the only change. :)

Thanks,
David


> 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