Bug 88474 – [GTK] run-api-tests should not buffer test stdout

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 146159 [details]
Patch

[Carlos Garcia Campos]

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.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 156338 [details]
Patch updated to apply on current git master

[Philippe Normand]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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().

[Philippe Normand]

Oh wow ok this parse_line thing confused me. Can it be renamed ro parse_line_callback for instance?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 164904 [details]
Updated patch according to review comments

[Carlos Garcia Campos]

Committed r129138: <http://trac.webkit.org/changeset/129138>