Not buffering stdout makes it easier to see why Google Tests time out (as you don't have to wait until the test times out to see the output) and preserves coloration of the output.
Created attachment 146159 [details] Patch
Comment on attachment 146159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146159&action=review Yes, I never liked either, but it's used to get the pid and kill the test in case of timeout. > Tools/gtk/run-api-tests:215 > child_pid = self._get_child_pid_from_test_output(stdout) stdout is used here, and you removed it. I'm fine with not buffering stdout but we need to figure out a way to kill the tests when they timeout.
(In reply to comment #2) > stdout is used here, and you removed it. I'm fine with not buffering stdout but we need to figure out a way to kill the tests when they timeout. Hrm. That's a really good point. Perhaps it would be sufficient to only capture stdout for GLib test programs. This change is most useful for the Google tests.
Created attachment 146277 [details] Another patch I think we could use select to read the output to avoid the buffering. We would still buffer lines, but we can send them to stdout when they are read, instead of waiting for the whole command output. And to preserve the colored output of google tests, we can use forkpty + exec instead of Popen.
Created attachment 156338 [details] Patch updated to apply on current git master
Comment on attachment 156338 [details] Patch updated to apply on current git master View in context: https://bugs.webkit.org/attachment.cgi?id=156338&action=review > Tools/ChangeLog:8 > + > + Nit, either provide a general description of the change or remove these empty lines :) > Tools/Scripts/run-gtk-tests:218 > + child_pid = [-1] What is the reason for using a list? From what I can see only its first item is used anyway. > Tools/gtk/common.py:130 > + parse_line(output[:pos + 1]) Where is this function defined? Also I'm not sure to understand the reason for this loop, can output.splitlines() simply be used?
(In reply to comment #6) > (From update of attachment 156338 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156338&action=review > > > Tools/ChangeLog:8 > > + > > + > > Nit, either provide a general description of the change or remove these empty lines :) Oh, sure. > > Tools/Scripts/run-gtk-tests:218 > > + child_pid = [-1] > > What is the reason for using a list? From what I can see only its first item is used anyway. It's a trick, since parameters are passed by value in python, using an integer wouldn't work, the value would be copied and the global variable is not updated. Using a list the pointer to the list is copied and updating the contents of the list affects the passed variable. > > Tools/gtk/common.py:130 > > + parse_line(output[:pos + 1]) > > Where is this function defined? Also I'm not sure to understand the reason for this loop, can output.splitlines() simply be used? At the beginning, it's the one getting the list you commented before. splitlines can't be used in this case, because we need to leave in the buffer anything after the last '\n'. splitlines would work after reading the whole output, but that's exactly what we are trying to avoid here.
(In reply to comment #6) > > Tools/gtk/common.py:130 > > + parse_line(output[:pos + 1]) > > Where is this function defined? The function is passed as parameter to parse_output_lines().
Oh wow ok this parse_line thing confused me. Can it be renamed ro parse_line_callback for instance?
(In reply to comment #9) > Oh wow ok this parse_line thing confused me. Can it be renamed ro parse_line_callback for instance? Sure.
Created attachment 164904 [details] Updated patch according to review comments
Committed r129138: <http://trac.webkit.org/changeset/129138>