Bug 69938 – stop spamming 'stopping test driver timed out, killing it' when running rwt

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

stop spamming 'stopping test driver timed out, killing it' when running rwt

[Tony Chang]

Created attachment 110694 [details]
Patch

[Tony Chang]

Comment on attachment 110694 [details]
Patch

Oh, I have a better idea.

[Tony Chang]

Created attachment 110695 [details]
Patch

[Dirk Pranke]

Comment on attachment 110695 [details]
Patch

So the intent is to give wait half as long as the default timeout, right?

options.time_out_ms is always non-zero (see run_webkit_tests.py:139). You could just as easily do:

kill_timeout_seconds = self._port.get_option('time_out_ms') / 2000.

[Tony Chang]

(In reply to comment #4)
> (From update of attachment 110695 [details])
> So the intent is to give wait half as long as the default timeout, right?

I was trying to wait 3 sec by default, regardless of what the default timeout is.

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 110695 [details] [details])
> > So the intent is to give wait half as long as the default timeout, right?
> 
> I was trying to wait 3 sec by default, regardless of what the default timeout is.

I see. Well, the 'else' branch won't ever execute, because of the previously cited code, so I don't think you can easily get that behavior. Per the discussion in bug 68026, it seems to me like half of the timeout value is a reasonable heuristic to use, though.

[Tony Chang]

Created attachment 110708 [details]
Patch for landing

[Tony Chang]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 110695 [details] [details] [details])
> > > So the intent is to give wait half as long as the default timeout, right?
> > 
> > I was trying to wait 3 sec by default, regardless of what the default timeout is.
> 
> I see. Well, the 'else' branch won't ever execute, because of the previously cited code, so I don't think you can easily get that behavior. Per the discussion in bug 68026, it seems to me like half of the timeout value is a reasonable heuristic to use, though.

I've removed the else branch, but don't understand what you mean by not being able to easily get the behavior of defaulting to 3 sec.  Any value for Manager.DEFAULT_TEST_TIMEOUT_MS should give us a 3 sec timeout.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 110695 [details] [details] [details] [details])
> > > > So the intent is to give wait half as long as the default timeout, right?
> > > 
> > > I was trying to wait 3 sec by default, regardless of what the default timeout is.
> > 
> > I see. Well, the 'else' branch won't ever execute, because of the previously cited code, so I don't think you can easily get that behavior. Per the discussion in bug 68026, it seems to me like half of the timeout value is a reasonable heuristic to use, though.
> 
> I've removed the else branch, but don't understand what you mean by not being able to easily get the behavior of defaulting to 3 sec.  Any value for Manager.DEFAULT_TEST_TIMEOUT_MS should give us a 3 sec timeout.

Oh, sorry, I think I was parsing your sentence differently. I was interpreting you to be saying that you wanted the kill value to default to 3 seconds regardless of whether someone had passed in a value via --time-out-ms, and I was saying that there was no way for you to tell if someone had used the command line switch or not. 

Of course, you couldn't have wanted that behavior, since the whole point of the prior change was to scale the kill value to the flag, and there isn't a way to specify the value directly, so fixing at that 3 seconds "by default" wouldn't have made sense.

My mistake. And you are correct that kill_timeout will be set to 3 regardless of the value of Manager.DEFAULT_TEST_TIMEOUT_MS.

[WebKit Review Bot]

Comment on attachment 110708 [details]
Patch for landing

Clearing flags on attachment: 110708

Committed r97293: <http://trac.webkit.org/changeset/97293>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.