Bug 88474 - [GTK] run-api-tests should not buffer test stdout
Summary: [GTK] run-api-tests should not buffer test stdout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-06 17:34 PDT by Martin Robinson
Modified: 2012-09-20 08:48 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2012-06-06 17:42 PDT, Martin Robinson
cgarcia: review-
Details | Formatted Diff | Diff
Another patch (5.20 KB, patch)
2012-06-07 06:38 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch updated to apply on current git master (5.23 KB, patch)
2012-08-03 05:50 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch according to review comments (5.28 KB, patch)
2012-09-20 06:04 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-06-06 17:34:50 PDT
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.
Comment 1 Martin Robinson 2012-06-06 17:42:07 PDT
Created attachment 146159 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-06-06 23:31:26 PDT
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.
Comment 3 Martin Robinson 2012-06-07 00:05:18 PDT
(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.
Comment 4 Carlos Garcia Campos 2012-06-07 06:38:05 PDT
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.
Comment 5 Carlos Garcia Campos 2012-08-03 05:50:54 PDT
Created attachment 156338 [details]
Patch updated to apply on current git master
Comment 6 Philippe Normand 2012-09-19 00:20:24 PDT
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?
Comment 7 Carlos Garcia Campos 2012-09-19 00:29:59 PDT
(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.
Comment 8 Carlos Garcia Campos 2012-09-19 00:33:25 PDT
(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().
Comment 9 Philippe Normand 2012-09-19 03:35:40 PDT
Oh wow ok this parse_line thing confused me. Can it be renamed ro parse_line_callback for instance?
Comment 10 Carlos Garcia Campos 2012-09-19 03:50:59 PDT
(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.
Comment 11 Carlos Garcia Campos 2012-09-20 06:04:02 PDT
Created attachment 164904 [details]
Updated patch according to review comments
Comment 12 Carlos Garcia Campos 2012-09-20 08:48:19 PDT
Committed r129138: <http://trac.webkit.org/changeset/129138>