[Fwd: SDK Test Fixes Batch for 2010.07 (6941287, 6962804, 6964018)]

[Daniel D. Daugherty]

Alan,

I'm adding the OpenJDK aliases back into this e-mail thread for
this reply. Just trying to keep folks in the loop...

Thanks for squeezing in time for a review of 6964018.


On 7/21/2010 1:43 PM, Alan Bateman wrote:
> Daniel D. Daugherty wrote:
>> On 7/21/2010 4:42 AM, Alan Bateman wrote:
>>> Daniel D. Daugherty wrote:
>>>> Alan,
>>>>
>>>> A quick look from you at the third webrev would be much appreciated...
>>>>
>>>> Dan
>>> Sorry Dan, I just don't have the cycles today due to a couple of 
>>> high priority issues.  I might get cycles on Friday if we really 
>>> want me to review.
>>
>> I may just go with only Kelly's review. :-|
> I just spent 15-20m going through 
> http://cr.openjdk.java.net/~dcubed/6964018-webrev/0/ in more detail 
> and it looks fine to me. Thanks for explaining about the refactoring 
> as it looks more than it really is on first glance.  I'm happy with 
> the updates to the existing tests and happy to see the new 
> test/java/util/logging tests are more reliable. Given that this is 
> tests only change then one reviewer should be fine.

I'll put both you and Kelly down for 6964018. I'll put just Kelly
down for the first two. Of course, that could change if I get more
reviewer comments.


> Minor nits in ShutdownSimpleApplication.java is that System.exit is 
> probably not needed.

Because I added the 'set -e' option, I think it is needed. Otherwise
I get a random process exit status.


> Also the pre-existing empty comment lines L25-26 can probably be removed.

I wondered why those were there so I left them in. I had meant to
ping you about them, but I forgot... I'll remove them.


> Same comments for SimpleApplication.

Hopefully, the same resolutions will be OK with you.


> BTW: In test/java/util/logging/LoggerWeakRefLeak.sh you can listed 
> issues with jmap - are they still valid with the updated test and the 
> recent fix to the attach API?

I suspect that your recent attach API fix will resolve those
issues, but I plan to specifically test for that after your
fix gets promoted.


> The only outstanding issue that I can think of is where you try to 
> attach just as the process is about to exit (but with the updated test 
> this doesn't happen due to the "handshake"/socket connection).

Agreed and thank you for pointing me in that direction.

Dan