[Webkit-unassigned] [Bug 38561] new-run-webkit-tests should check each thread for exceptions instead of just the first one

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38561





--- Comment #10 from Eric Seidel <eric at webkit.org>  2010-05-05 19:28:33 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.