[test] RFR 8209771: jdk.test.lib.Utils::runAndCheckException error

[Weijun Wang]

Hi David

> On Aug 21, 2018, at 1:02 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Max,
> 
> On 21/08/2018 12:53 PM, Weijun Wang wrote:
>> Please take a review at
>>    https://bugs.openjdk.java.net/browse/JDK-8209771
>>    http://cr.openjdk.java.net/~weijun/8209771/webrev.00/
>> There is an error in the implementation of the method, and I also think ThrowingRunnable is more developer friendly here.
> 
> ThrowingRunnable is really only needed for a Runnable that might throw checked exceptions. It really doesn't make any difference here.

By using ThrowingRunnable I can put a one-line lambda expression that might throw a checked exception, otherwise I'll have to wrap it into a RuntimeException. That's what I meant developer-friendly.

> 
> The actual logic fix seems okay. You could initialize throwable to null and avoid the assignment in the try block.

Sure.

> 
>> The methods are only used in hotspot so I'm asking for the code review here.
> 
> Mostly by the JVMCI compiler folk too.

In fact, mostly by tests in hotspot/jtreg/compiler/jvmci. I thought it's a part of hotspot. Anyway, I've added hotspot-compiler-dev at .

> I didn't even know these utilities existed. I've written tons of test code that checks for expected and unexpected exceptions. :)

Me too. This morning I had a little free time to wander around jdk.test.lib and notice these.

> 
>> I also think in filterException(), filter.apply(null) should be called if test.run() has not thrown any exception, but this is a behavior change and I cannot be sure.
> 
> That would be a different function I think. filterException seems to me just to be a generalized catch clause to detect if a specific kind of exception occurred - not to check that some specific exception should have occurred.

Maybe. I won't touch it.

Thanks
Max

> 
> Thanks,
> David
> 
>> Please include my name in the recipient list as I'm not in the mail list.
>> This will be a noreg-self.
>> Thanks
>> Max
>>