WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
88474
[GTK] run-api-tests should not buffer test stdout
https://bugs.webkit.org/show_bug.cgi?id=88474
Summary
[GTK] run-api-tests should not buffer test stdout
Martin Robinson
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Martin Robinson
Comment 1
2012-06-06 17:42:07 PDT
Created
attachment 146159
[details]
Patch
Carlos Garcia Campos
Comment 2
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.
Martin Robinson
Comment 3
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.
Carlos Garcia Campos
Comment 4
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.
Carlos Garcia Campos
Comment 5
2012-08-03 05:50:54 PDT
Created
attachment 156338
[details]
Patch updated to apply on current git master
Philippe Normand
Comment 6
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?
Carlos Garcia Campos
Comment 7
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.
Carlos Garcia Campos
Comment 8
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().
Philippe Normand
Comment 9
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?
Carlos Garcia Campos
Comment 10
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.
Carlos Garcia Campos
Comment 11
2012-09-20 06:04:02 PDT
Created
attachment 164904
[details]
Updated patch according to review comments
Carlos Garcia Campos
Comment 12
2012-09-20 08:48:19 PDT
Committed
r129138
: <
http://trac.webkit.org/changeset/129138
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug