Bug 69938 - stop spamming 'stopping test driver timed out, killing it' when running rwt
Summary: stop spamming 'stopping test driver timed out, killing it' when running rwt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-12 10:02 PDT by Tony Chang
Modified: 2011-10-12 13:24 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2011-10-12 10:04 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (2.32 KB, patch)
2011-10-12 10:09 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (2.27 KB, patch)
2011-10-12 11:11 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-10-12 10:02:44 PDT
stop spamming 'stopping test driver timed out, killing it' when running rwt
Comment 1 Tony Chang 2011-10-12 10:04:48 PDT
Created attachment 110694 [details]
Patch
Comment 2 Tony Chang 2011-10-12 10:05:12 PDT
Comment on attachment 110694 [details]
Patch

Oh, I have a better idea.
Comment 3 Tony Chang 2011-10-12 10:09:40 PDT
Created attachment 110695 [details]
Patch
Comment 4 Dirk Pranke 2011-10-12 10:44:36 PDT
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.
Comment 5 Tony Chang 2011-10-12 10:56:00 PDT
(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.
Comment 6 Dirk Pranke 2011-10-12 11:01:33 PDT
(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.
Comment 7 Tony Chang 2011-10-12 11:11:49 PDT
Created attachment 110708 [details]
Patch for landing
Comment 8 Tony Chang 2011-10-12 11:14:03 PDT
(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.
Comment 9 Dirk Pranke 2011-10-12 11:21:53 PDT
(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.
Comment 10 WebKit Review Bot 2011-10-12 13:24:02 PDT
Comment on attachment 110708 [details]
Patch for landing

Clearing flags on attachment: 110708

Committed r97293: <http://trac.webkit.org/changeset/97293>
Comment 11 WebKit Review Bot 2011-10-12 13:24:06 PDT
All reviewed patches have been landed.  Closing bug.