Reland: Take 2: Force python test server output to be unbuffered, so it doesn't mix with gtest outpu

Reland: Take 2: Force python test server output to be unbuffered, so it doesn't mix with gtest output

In browser tests that use a local python server, the python output in
the test logs sometimes overlaps with gtest output, resulting in gtest
falsely detecting passing tests as incomplete. This is a result of
python's default use of buffered output, which gets written to the log
file out of order.

This patch forces the python process for local test servers to use
unbuffered mode via the -u switch. This way, by the time gtest is ready
to log a passing test, all testserver output is already written to the
log file.

Update: The win xp bots were adversely affected by the -u switch, so we now
apply it only for mac and linux, which is where the original problem was found.

BUG=147368
TEST=See sync integration test output when it is redirected to a log file, and make sure there are no false negatives.

Originally committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156361

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156389

[Raghu Simha, 2012-09-10 23:43:33]

Pawel, please review.

Note: I had to revert the previous attempt at this fix (involving setting the
PYTHONUNBUFFERED env variable) because it seemed to be messing with xp results.

I looked for a way to set the -u flag in one place, but there doesn't seem to be
a good spot. There's precedent for using -u in
chrome/browser/safe_browsing/safe_browsing_test.cc, and I've used the same
method all over the place.

Let me know what you think.

[Paweł Hajdan Jr., 2012-09-11 08:57:30]

http://codereview.chromium.org/10907162/diff/1/content/public/test/browser_te...
File content/public/test/browser_test_utils.cc (right):

http://codereview.chromium.org/10907162/diff/1/content/public/test/browser_te...
content/public/test/browser_test_utils.cc:550: cmd_line->AppendArg("-u");
Why not in CreatePythonCommandLine?

In fact, to reduce code duplication, how about changing GetPythonRunTime from
returning FilePath to returning CommandLine, and using the -u switch there? You
could add a parameter to control it, e.g. STDIO_BUFFERED, STDIO_UNBUFFERED.

[Raghu Simha, 2012-09-11 21:58:04]

Pawel, PTAL.

http://codereview.chromium.org/10907162/diff/1/content/public/test/browser_te...
File content/public/test/browser_test_utils.cc (right):

http://codereview.chromium.org/10907162/diff/1/content/public/test/browser_te...
content/public/test/browser_test_utils.cc:550: cmd_line->AppendArg("-u");
On 2012/09/11 08:57:31, Paweł Hajdan Jr. wrote:
> Why not in CreatePythonCommandLine?
> 
> In fact, to reduce code duplication, how about changing GetPythonRunTime from
> returning FilePath to returning CommandLine, and using the -u switch there?

Done.

> You could add a parameter to control it, e.g. STDIO_BUFFERED,
STDIO_UNBUFFERED.

Can you think of any place where we will want to use STDIO_BUFFERED? From what I
understand, we will need to add the -u arg to python at every place where a
chrome test invokes the python runtime, i.e., at every place where
GetPythonRunTime is called.

[Raghu Simha, 2012-09-12 00:10:05]

Looks like we have more widespread problems than just testserver output. For
example, see the win_rel trybot logs:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Open the stdio for content_browsertests and search for the truncated string
"IndexedDBBrowserTest.TransactiNote"

I'm not sure what the ideal solution is. One thing I can think of is to add the
-u string to all the gtest and testserver python files, and that way, they will
run in unbuffered mode, and we won't need all the changes in this CL.

Let me know what you think.

[Raghu Simha, 2012-09-12 00:11:53]

What I mean is to use this as the first line in all gtest and testserver python
scripts.

"#!/usr/bin/python -u"

[Paweł Hajdan Jr., 2012-09-12 10:37:25]

This LGTM with comments.

I'm not sure if #!/usr/bin/python -u would help - we launch the scripts via
python binary anyway I guess because Windows doesn't recognize shebangs.

http://codereview.chromium.org/10907162/diff/6003/content/public/test/browser...
File content/public/test/browser_test_utils.cc (right):

http://codereview.chromium.org/10907162/diff/6003/content/public/test/browser...
content/public/test/browser_test_utils.cc:518:
CHECK(GetPythonRunTime(cmd_line));
nit: Please add a TODO(phajdan.jr) here to convert the CHECK to bool return
value. We shouldn't be crashing tests on error.

http://codereview.chromium.org/10907162/diff/6003/net/test/python_utils.h
File net/test/python_utils.h (right):

http://codereview.chromium.org/10907162/diff/6003/net/test/python_utils.h#new...
net/test/python_utils.h:23: bool GetPythonRunTime(CommandLine* python_cmd)
WARN_UNUSED_RESULT;
nit: Please also rename the function, GetPythonRunTime -> GetPythonCommand or
GetPythonCommandLine (you had to change all call sites anyway).

I'm fine with always using unbuffered output.

[Raghu Simha, 2012-09-12 18:31:20]

Nits fixed. Landing via CQ.

http://codereview.chromium.org/10907162/diff/6003/content/public/test/browser...
File content/public/test/browser_test_utils.cc (right):

http://codereview.chromium.org/10907162/diff/6003/content/public/test/browser...
content/public/test/browser_test_utils.cc:518:
CHECK(GetPythonRunTime(cmd_line));
On 2012/09/12 10:37:25, Paweł Hajdan Jr. wrote:
> nit: Please add a TODO(phajdan.jr) here to convert the CHECK to bool return
> value. We shouldn't be crashing tests on error.

Done.

http://codereview.chromium.org/10907162/diff/6003/net/test/python_utils.h
File net/test/python_utils.h (right):

http://codereview.chromium.org/10907162/diff/6003/net/test/python_utils.h#new...
net/test/python_utils.h:23: bool GetPythonRunTime(CommandLine* python_cmd)
WARN_UNUSED_RESULT;
On 2012/09/12 10:37:25, Paweł Hajdan Jr. wrote:
> nit: Please also rename the function, GetPythonRunTime -> GetPythonCommand or
> GetPythonCommandLine (you had to change all call sites anyway).
> 
> I'm fine with always using unbuffered output.

Done.

[commit-bot: I haz the power, 2012-09-12 18:50:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/10907162/9

[commit-bot: I haz the power, 2012-09-12 18:50:38]

Presubmit check for 10907162-9 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/safe_browsing

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Raghu Simha, 2012-09-12 18:52:44]

+shess for chrome/browser/safe_browsing/safe_browsing_test.cc owner approval.

[Scott Hess - ex-Googler, 2012-09-12 19:41:05]

lgtm for safe_browsing.