Bug 38561 – new-run-webkit-tests should check each thread for exceptions instead of just the first one

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

new-run-webkit-tests should check each thread for exceptions instead of just the first one

[Eric Seidel (no email)]

Created attachment 55087 [details]
work in progress patch

[Ojan Vafai]

Comment on attachment 55087 [details]
work in progress patch

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:246
 +  class ThreadTimings(object):
I really like that you've made this a class. I'm a bit sad that you've deleted the comments explaining what each of the member variables are. I had to go read the old comments to confirm this code is  correct.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:675
 +              print "\nlogging._lock: %s\n" % logging._lock
Probably should use logging.debug?

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:705
 +                      # and any not-yet-run tests should cannot be assumed to pass.
should cannot?

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:706
 +                      thread.reraise_exception()
This is a change in behavior. It means we fail early if a thread has an error. I think that's better though.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:712
 +                  # If we got any exception, tear down all threads.
We're in a finally clause here. We don't know that we got an exception. I think you mean this to be an except clause?

[Eric Seidel (no email)]

(In reply to comment #2)
> I really like that you've made this a class. I'm a bit sad that you've deleted
> the comments explaining what each of the member variables are. I had to go read
> the old comments to confirm this code is  correct.

Agreed.  Will add them back.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:675
>  +              print "\nlogging._lock: %s\n" % logging._lock
> Probably should use logging.debug?

No.  I intentionally use print here.  1.  To match the other thread dumping code, and 2.  to avoid hitting the taking the logging lock which could be held by some other thread.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:705
>  +                      # and any not-yet-run tests should cannot be assumed to
> pass.
> should cannot?

Will fix.  Was trying to re-write the comment to be fewer lines.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:706
>  +                      thread.reraise_exception()
> This is a change in behavior. It means we fail early if a thread has an error.
> I think that's better though.

Yup.  That one line is the whole point of the patch. :)

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:712
>  +                  # If we got any exception, tear down all threads.
> We're in a finally clause here. We don't know that we got an exception. I think
> you mean this to be an except clause?

No, this is also intentional.  Basically if you ever leave that while loop, all threads should be canceled.  That code makes sure that's the case.  I could except, cancel and then re raise, that would be equivalent since thread.cancel() on a completed thread does nothing.

[Ojan Vafai]

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:712
> >  +                  # If we got any exception, tear down all threads.
> > We're in a finally clause here. We don't know that we got an exception. I think
> > you mean this to be an except clause?
> 
> No, this is also intentional.  Basically if you ever leave that while loop, all
> threads should be canceled.  That code makes sure that's the case.  I could
> except, cancel and then re raise, that would be equivalent since
> thread.cancel() on a completed thread does nothing.

Maybe I'm missing something. For this to work right, doesn't the while loop itself need to be inside the try instead of the try being in the while loop?

[Eric Seidel (no email)]

> Maybe I'm missing something. For this to work right, doesn't the while loop
> itself need to be inside the try instead of the try being in the while loop?

Yes.  There are several other typos in the first patch. A working one coming shortly. :)

[Eric Seidel (no email)]

Created attachment 55177 [details]
Patch

[Ojan Vafai]

Comment on attachment 55177 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:703
 +              # and any not-yet-run tests should cannot be assumed to pass.
Still have this non-sensical comment. Please fix grammar.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:708
 +              thread_finished_callback=_thread_finished)
Did you really write this patch? Wrapping at 80 columns ==> imposter Eric. :)

Code all looks good to me.

[Eric Seidel (no email)]

Oh, I wrap to 80c when it makes the code more readable.  I just hate having the blanket rule which produces crap wrapping so often. :)

[Dirk Pranke]

Comment on attachment 55177 [details]
Patch

There are a couple of seemingly unrelated changes in this patch. Several lines have been changed because you declared command line arguments as int; that's a good change, but please note it in the ChangeLog.

Also, there seems to be an unrelated change in websocket_server.py; while it's good to fix bugs, it might be better to put this into a different patch in case we have to roll one or the other of these things back.

ThreadTimings should probably be moved (along with TestResult and ResultSummary) into a separate module soonish.

Also, ThreadTimings is a bad name, since two of the three parameters (directory_test_tiings and individual_test_timings) have nothing to do with threads. Maybe TimingSummary() to match ResultSummary() or something?

I wonder if we'd be better off calling time.sleep() rather than thread.join() during the main loop, and just doing thread.join() when we know all the tests have been executed. I guess you'd need some other way to figure out if the thread had bailed out with an exception.

I also think having ThreadWatcher extract and raise the exception information directly would be clearer rather than hiding that in methods in dump_render_tree_thread. Somehow reading thread.reraise_exception() makes me think you're raising an exception on a different thread, rather than pulling the exception info off of the other thread and raising it on the current thread.

Note that the "starting testing..." print statement on line 710 is inaccurate - testing may have already started in other threads, and in fact will be complete if you are running single-threaded. In my latest printing patch I changed this to "all threads started" which is slightly more accurate.

On line 713 of run_webkit_tests.py you deleted the call to self.update_summary() that happens right before we return. you previously had the comment that calls were made from the individual threads themselves. That is only true in the case where we're running single-threaded and we call run_in_main_thread(). I think that whole code path might be clearer if we moved the run_in_main_thread() code out of _instantiate_test_shell_threads and into _run(), and then explicitly call update_summary() after run_in_main_thread (although this could probably all be deferred to a later patch).

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 55177 [details])
> There are a couple of seemingly unrelated changes in this patch. Several lines
> have been changed because you declared command line arguments as int; that's a
> good change, but please note it in the ChangeLog.

I'm happy to split them out or talk about them in the ChangeLog, or both. :)

> Also, there seems to be an unrelated change in websocket_server.py; while it's
> good to fix bugs, it might be better to put this into a different patch in case
> we have to roll one or the other of these things back.

Yeah, I just hit the exception while trying to run new-run-webkit-tests.  Again, happy to do it in a separate change.

> ThreadTimings should probably be moved (along with TestResult and
> ResultSummary) into a separate module soonish.

Agreed.

> Also, ThreadTimings is a bad name, since two of the three parameters
> (directory_test_tiings and individual_test_timings) have nothing to do with
> threads. Maybe TimingSummary() to match ResultSummary() or something?

However you like.  It's not a very good class.  It's just marginally better than the tuple we were passing around.

> I wonder if we'd be better off calling time.sleep() rather than thread.join()
> during the main loop, and just doing thread.join() when we know all the tests
> have been executed. I guess you'd need some other way to figure out if the
> thread had bailed out with an exception.

Yeah that wouldn't work. This is re: the comment that join() will fail if the thread hasn't been started?  That shouldn't be an issue in practice, but I did hit it when running the tests as they tested main-thread mode (which, btw is a total grotesque hack).

> I also think having ThreadWatcher extract and raise the exception information
> directly would be clearer rather than hiding that in methods in
> dump_render_tree_thread. Somehow reading thread.reraise_exception() makes me
> think you're raising an exception on a different thread, rather than pulling
> the exception info off of the other thread and raising it on the current
> thread.

ThreadWatcher doesn't (and shoudn't) know about DumpRenderTreeThread (which seems poorly named now that I know what it does).  The thread has to know how to store off the exception info, so it seemed fitting that it know how to raise it (since it can't return it in any condensed form as raise requires 3 distinct arguments).  We could abstract the exception saving into some thread subclass which DRTThread inherited from?  But I think we could do that in a separate change.

> Note that the "starting testing..." print statement on line 710 is inaccurate -
> testing may have already started in other threads, and in fact will be complete
> if you are running single-threaded. In my latest printing patch I changed this
> to "all threads started" which is slightly more accurate.

True.  I added that log statement to help better explain to the user which states we were in.  It also made clear that we have some strange delay after starting the threads before any of them return any data (we don't get any updates until about 2% testing is done).

You're right that it's wrong for single-threaded, but again that's because single-threaded hacks in at a bad place IMO.

I'll change the log to match whatever you changed it to in your other patch.

> On line 713 of run_webkit_tests.py you deleted the call to
> self.update_summary() that happens right before we return. you previously had
> the comment that calls were made from the individual threads themselves. That
> is only true in the case where we're running single-threaded and we call
> run_in_main_thread().

Correct, that comment was partially wrong.  We definitely don't need that update.  ThreadWatcher will always call the thread_finished callback when a thread finishes, so we will always update after a thread is done (including the last thread).

> I think that whole code path might be clearer if we moved
> the run_in_main_thread() code out of _instantiate_test_shell_threads and into
> _run(), and then explicitly call update_summary() after run_in_main_thread
> (although this could probably all be deferred to a later patch).

YES!  I totally agree.  run_in_main_thread is a $DIETY-forsaken hack.

[Eric Seidel (no email)]

Created attachment 55197 [details]
fix ungrammar and ChangeLog

[Dirk Pranke]

I have a patch that modifies this slightly. I will upload it, and then Eric can review it and this can go away ;)

[Dirk Pranke]

Created attachment 56960 [details]
alternate approach at reading status from all of the threads

I'm uploading an alternate approach to fixing this bug for comparison.

Some notes:

- I didn't create either a ThreadTimings class or a ThreadWatcher class, but I did abstract the thread watching loop into a separate standalone function (w/o unit tests as yet).

- I didn't fix the bugs in websocket_server or the command line parsing argument handling

The major difference (and contribution of this patch) is that this patch explicitly watches for progress on each thread, and logs a stack trace if it looks like the thread is hung. Arguably, we should also raise an assertion and bail out at this point (or figure out a way to kill and restart the thread).

I think the "right" solution is to merge this patch with Eric's in some way, but I'm not sure what the best way to do that is so I'm posting this as-is so that we can compare and contrast the two approaches. I will submit more feedback on Eric's patch in a separate comment.

[Dirk Pranke]

(In reply to comment #11)
> Created an attachment (id=55197) [details]
> fix ungrammar and ChangeLog

So, more comments on this latest patch.

(1) You didn't address my comment that "ThreadTimings" was a misleading name; something like "TimingStatistics" or "ResultTimings" would be better. I also think that introducing this class to replace the tuple doesn't really help that much - you add ~20 lines of code to replace something that is used once. Would replacing the tuple with a hash be an acceptable solution?

(2) Regarding my comment on time.sleep() instead of thread.join() - I think an important point is that we don't really care about doing a join() - essentially I think we're using join as a stand-in for sleep. 

Your patch, because it's "round-robin sleeping" through the list of threads, does help us detect exceptions faster than the original code, but still not as fast as it could. Best case it takes us O( # of threads * amount of wake_every) time to detect that the last thread immediately raised an exception. Given that wake_every is small, this isn't a big deal.

In contrast, the patch I just uploaded checks every thread for exceptions after a single join/sleep. There may be some locking issues with my patch, but I think the usage pattern is probably safe enough (we could put a lock on it to be sure).

(3) Regarding "having ThreadWatcher extract and raise the exception information", I think I failed to get my point across and was unnecessarily confusing. My point was intended to be about what we're doing in reraise_exception() and the calling routine, not in ThreadWatcher. I agree that ThreadWatcher shouldn't know anything about the other class and that part of the code is fine.

What I meant to say is that in the thread_finished callback, I think it is clearer to write that as (pseudocode):

def thread_finished(thread):
  exc = thread.exception_info()
  if exc:
    raise exc

because that makes it clear(er) what is being done on which thread. I found the reraise_exception() routine to be confusing because the name makes it sound like it will always raise an exception. Also, the docstring is confusing at best, and you shouldn't do a _log.info() for the normal termination case (_log.debug() would be better).

If you did want to keep that logic in the dump_render_thread class, can we at least change the name to something (admittedly awkward) like reraise_exception_if_necessary() ?

(4) The major advantage of creating a separate ThreadWatcher class seems to be that you can more easily test it with Mock objects (since it's not like this code is being reused anywhere). I have to admit that I'm somewhat uneasy with using Mock objects here for the way you're testing this, since you're not actually creating threads and that seems to me to introduce a fundamental uncertainty into whether or not you're really testing the behavior that we'll see in the real world. I'm also not sure that the increased testability offsets the decrease in readability caused by the new abstraction, but I'm definitely on the fence here.

[Adam Barth]

Comment on attachment 55197 [details]
fix ungrammar and ChangeLog

Eric says this patch probably doesn't even apply any more.  If you'd like a more detailed review, please rebase this patch to top-of-tree.  Thanks.

[Dirk Pranke]

Created attachment 59461 [details]
Patch

[Dirk Pranke]

I just uploaded a new version of my patch that applies against (current) ToT. It still doesn't have unit tests, but is otherwise okay. Please review and, if we like this approach better than Eric's, I can write some tests.

[Ojan Vafai]

Eric, I think this is waiting on feedback from you. I don't have strong opinions about either approach. I'd be OK with r+'ing either patch.

[Eric Seidel (no email)]

Comment on attachment 59461 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:253
 +      def get_timeout_padding(self, timeout):
Do we really use get_ in webkit?  Does PEP8 suggest it?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:271
 +          self._thread_id = thread.get_ident()
No clue if this will work correctly on Win32.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:638
 +              if thread_id == id:
Do we expect this compare to work correctly on win32?

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:644
 +          _log.error("")
Should we assert here?  That we never found a thread? Maybe we should have the "find the right thread from this list" function be separate?

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:684
 +                      exception_info = thread.get_exception_info()
It's a bit strange to have this information outside of the DRT thread.  Seems a raise_if_needed() method would be "cleaner", but OK.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:698
 +                              thread.clear_next_timeout()
Do we need to kill the thread? make a new one?  Does clear_next_timeout do that?

[Dirk Pranke]

(In reply to comment #19)
> (From update of attachment 59461 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:253
>  +      def get_timeout_padding(self, timeout):
> Do we really use get_ in webkit?  Does PEP8 suggest it?
> 

no, not often. PEP8 would suggest making this an attribute and/or hiding it under __getattr__. I can make that change if desired.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:271
>  +          self._thread_id = thread.get_ident()
> No clue if this will work correctly on Win32.
> 

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:638
>  +              if thread_id == id:
> Do we expect this compare to work correctly on win32?
>

I believe these work on win32, but I can retest. It's now been quite a while since I tried this patch.
 
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:644
>  +          _log.error("")
> Should we assert here?  That we never found a thread? Maybe we should have the "find the right thread from this list" function be separate?
> 

You mean, assert if we didn't find the thread? (Currently, we print nothing). I can make that change but I don't feel strongly about it.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:684
>  +                      exception_info = thread.get_exception_info()
> It's a bit strange to have this information outside of the DRT thread.  Seems a raise_if_needed() method would be "cleaner", but OK.
>

Yeah, that was what your patch did. I felt it was cleaner to pull the information across into the caller and raise the thread in the caller, rather than make something in the DRT class raise an exception that wasn't on the DRT thread.
 
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:698
>  +                              thread.clear_next_timeout()
> Do we need to kill the thread? make a new one?  Does clear_next_timeout do that?

It's unclear if you can actually kill the thread (portably), since it's wedged. All clear_next_timeout does is take it out of the list of threads we're watching, so that we don't keep repeating the thread stacks.

It would be possible to extend this patch (or do a follow-on one) that actually killed and restarted the tests to unwedge things. I would prefer to fix the code so that we don't get wedged in the first place, whicih is why I haven't written this code. Of course, I also thought I would've had time to fix the code months ago ;)

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 59461 [details] [details])
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:253
> >  +      def get_timeout_padding(self, timeout):
> > Do we really use get_ in webkit?  Does PEP8 suggest it?
> > 
> no, not often. PEP8 would suggest making this an attribute and/or hiding it under __getattr__. I can make that change if desired.

I think we should avoid naming things with get_ since that matches other code.  I've never used __getattr__, but if that's the clean-python-way, then more power to you!

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:271
> >  +          self._thread_id = thread.get_ident()
> > No clue if this will work correctly on Win32.
> > 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:638
> >  +              if thread_id == id:
> > Do we expect this compare to work correctly on win32?
> >
> I believe these work on win32, but I can retest. It's now been quite a while since I tried this patch.

If it doesn't, we can fix it later.  Just checking.  I know thread ids are not x-platform.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:644
> >  +          _log.error("")
> > Should we assert here?  That we never found a thread? Maybe we should have the "find the right thread from this list" function be separate?
> > 
> 
> You mean, assert if we didn't find the thread? (Currently, we print nothing). I can make that change but I don't feel strongly about it.

Yeah, I think that we should separate this out into a separate function and we can ASSERT in the caller that the function returned a thread (since it should always assuming our thread_id stuff is working on all relevant platforms, right?).

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:684
> >  +                      exception_info = thread.get_exception_info()
> > It's a bit strange to have this information outside of the DRT thread.  Seems a raise_if_needed() method would be "cleaner", but OK.
> >
> Yeah, that was what your patch did. I felt it was cleaner to pull the information across into the caller and raise the thread in the caller, rather than make something in the DRT class raise an exception that wasn't on the DRT thread.

Yeah.  It's not a big deal.  The tradeoff I was going for was trying to contain the knowledge of how to save and rebuild an exception inside the DRTThread.  Another way to do it would be to have the DRTThread expose a saved_exception object and then that object could have a reraise method or similar.  I think it's kinda poor design of this (relatively low-level) python API to make you use tuples like they do.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:698
> >  +                              thread.clear_next_timeout()
> > Do we need to kill the thread? make a new one?  Does clear_next_timeout do that?
> 
> It's unclear if you can actually kill the thread (portably), since it's wedged. All clear_next_timeout does is take it out of the list of threads we're watching, so that we don't keep repeating the thread stacks.
> 
> It would be possible to extend this patch (or do a follow-on one) that actually killed and restarted the tests to unwedge things. I would prefer to fix the code so that we don't get wedged in the first place, whicih is why I haven't written this code. Of course, I also thought I would've had time to fix the code months ago ;)

Sounds good. :)  We can always do that in a later patch.

[Dirk Pranke]

Okay, I will update the patch, re-verify on windows, and resubmit. Thanks!

[Dirk Pranke]

Created attachment 63965 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 63965 [details]
Patch

I'm confused as to why WatchableThread defines _thread_id but never sets it.  And then TestShellThread does?  Isn't that sorta an abstraction violation?

[Eric Seidel (no email)]

Comment on attachment 63965 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:229
 +          else:
No need for else after return.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1675
 +      found_stack = None
This local seems unneeded.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:278
 +          return float(timeout) * 3 / 1000
Huh?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:272
 +      def timeout_padding(self, timeout):
Seems like a strange name for what this does.
it's giving you a hung_test_timeout or similar?  Why a function? isn't this operating on a constant?

Why do we want .003 * timeout there?


I'm confused by the .003 timeout stuff.

[Dirk Pranke]

(In reply to comment #25)
> (From update of attachment 63965 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:229
>  +          else:
> No need for else after return.
> 

Fixed.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1675
>  +      found_stack = None
> This local seems unneeded.
>

I'm not sure what you mean by this. You could rewrite the function as:

def _find_thread_stack(id):
    for thread_id, stack in ...:
        if thread_id == id:
            return stack
    return None
 
 is that what you were suggesting? I did it this way to enable myself to get 100% line coverage with a single test; arguably I should really have two tests, but given that you should never really hit the "return None" case in real life, it seemed like an artificial sort of test and I thought this might be better.

WDYT?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:278
>  +          return float(timeout) * 3 / 1000
> Huh?
> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:272
>  +      def timeout_padding(self, timeout):
> Seems like a strange name for what this does.
> it's giving you a hung_test_timeout or similar?  Why a function? isn't this operating on a constant?
> 
> Why do we want .003 * timeout there?
> 
> 
> I'm confused by the .003 timeout stuff.


Which part of this was confusing? The * 3, or the / 1000 ? We multiple by three in order to make sure we don't timeout earlier than DRT itself. We divide by 1000 because python uses floating point seconds and DRT uses integer milliseconds. 

Should I add a comment explaining the divide, or do you want a different way of rounding up to give DRT a little headroom?

[Eric Seidel (no email)]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 63965 [details] [details])
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:229
> >  +          else:
> > No need for else after return.
> > 
> 
> Fixed.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1675
> >  +      found_stack = None
> > This local seems unneeded.
> >
> 
> I'm not sure what you mean by this. You could rewrite the function as:
> 
> def _find_thread_stack(id):
>     for thread_id, stack in ...:
>         if thread_id == id:
>             return stack
>     return None

Yes, that's what I meant.

>  is that what you were suggesting? I did it this way to enable myself to get 100% line coverage with a single test; arguably I should really have two tests, but given that you should never really hit the "return None" case in real life, it seemed like an artificial sort of test and I thought this might be better.

I guess I don't understand how those differ in line coverage.

> WDYT?
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:278
> >  +          return float(timeout) * 3 / 1000
> > Huh?
> > 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:272
> >  +      def timeout_padding(self, timeout):
> > Seems like a strange name for what this does.
> > it's giving you a hung_test_timeout or similar?  Why a function? isn't this operating on a constant?
> > 
> > Why do we want .003 * timeout there?
> > 
> > 
> > I'm confused by the .003 timeout stuff.
> 
> 
> Which part of this was confusing? The * 3, or the / 1000 ? We multiple by three in order to make sure we don't timeout earlier than DRT itself. We divide by 1000 because python uses floating point seconds and DRT uses integer milliseconds. 

Yes, this just want's clear.  Maybe named constants is what' you're looking for?  debugTimeoutMultiplier and millisecondsToSeconds?  I was confused by the line, yes.

[Dirk Pranke]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 63965 [details] [details] [details])
> > > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:229
> > >  +          else:
> > > No need for else after return.
> > > 
> > 
> > Fixed.
> > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1675
> > >  +      found_stack = None
> > > This local seems unneeded.
> > >
> > 
> > I'm not sure what you mean by this. You could rewrite the function as:
> > 
> > def _find_thread_stack(id):
> >     for thread_id, stack in ...:
> >         if thread_id == id:
> >             return stack
> >     return None
> 
> Yes, that's what I meant.
> 
> >  is that what you were suggesting? I did it this way to enable myself to get 100% line coverage with a single test; arguably I should really have two tests, but given that you should never really hit the "return None" case in real life, it seemed like an artificial sort of test and I thought this might be better.
> 
> I guess I don't understand how those differ in line coverage.
> 

In the code in the patch, every line is executed. In the code above, in the normal case, the "return None" line doesn't execute. There are two different paths in both cases, and only one is executed. 

I guess I'll change it to reflect reality better and stop being lazy :)

> > WDYT?
> > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:278
> > >  +          return float(timeout) * 3 / 1000
> > > Huh?
> > > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:272
> > >  +      def timeout_padding(self, timeout):
> > > Seems like a strange name for what this does.
> > > it's giving you a hung_test_timeout or similar?  Why a function? isn't this operating on a constant?
> > > 
> > > Why do we want .003 * timeout there?
> > > 
> > > 
> > > I'm confused by the .003 timeout stuff.
> > 
> > 
> > Which part of this was confusing? The * 3, or the / 1000 ? We multiple by three in order to make sure we don't timeout earlier than DRT itself. We divide by 1000 because python uses floating point seconds and DRT uses integer milliseconds. 
> 
> Yes, this just want's clear.  Maybe named constants is what' you're looking for?  debugTimeoutMultiplier and millisecondsToSeconds?  I was confused by the line, yes.

Okay, I will try to clarify this.

[Dirk Pranke]

Created attachment 64373 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 64373 [details]
Patch

rs=me.

[Dirk Pranke]

Comment on attachment 64373 [details]
Patch

Clearing flags on attachment: 64373

Committed r65343: <http://trac.webkit.org/changeset/65343>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65343 might have broken SnowLeopard Intel Release (Tests)