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

Tue Oct 21 20:08:53 UTC 2014

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
>>>>>>
>>>>>>
>>>>>
>>>>
>>
>