Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time

Update run_gtest_perf_test.py to streaming command output to stdout & log file at the same time

BUG=723821
TBR=dpranke@chromium.org

Review-Url: https://codereview.chromium.org/2889153007
Cr-Commit-Position: refs/heads/master@{#473376}
Committed: https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e40fed8db78e6

[nednguyen, 2017-05-19 19:48:10]

Patchset #1 (id:1) has been deleted

[nednguyen, 2017-05-19 19:48:19]

nednguyen@google.com changed reviewers:
+ dpranke@chromium.org

[nednguyen, 2017-05-19 19:50:17]

Description was changed from

==========
Update run_gtest_perf_test.py to streaming command output to stdout & log file
at the same time

BUG=723821
==========

to

==========
Update run_gtest_perf_test.py to streaming command output to stdout & log file
at the same time

BUG=723821
TBR=dpranke@chromium.org
==========

[nednguyen, 2017-05-19 19:50:21]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2017-05-19 19:51:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nednguyen, 2017-05-19 20:02:10]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2017-05-19 20:07:18]

nednguyen@google.com changed reviewers:
+ dnj@chromium.org

[Dirk Pranke, 2017-05-19 20:09:29]

I defer to dnj@ here. lgtm if/when he approves.

[dnj, 2017-05-19 20:56:19]

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:78: rc = 1
No point setting this here.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:81: stdoutfile_header = open(stdoutfile, 'w')
nit: do this in try/finally clause, closing on finally.

Alternatively, you can use "with" and a special contextmanager:

@contextlib.contextmanager
def maybe_open(path, mode):
  if not path:
    yield None
    return

  with open(path, mode) as fd:
    yield fd

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:82: process = subprocess.Popen(argv, env=env, cwd=cwd,
stdout=subprocess.PIPE,
Note that before, if the argv[0] did not exist or wasn't executable, you would
catch that in the "except Exception" block; now, it will raise an Exception. Is
this OK?

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:84: for c in iter(lambda: process.stdout.read(1), ''):
How much data are we expecting here? Reading byte-by-byte and blocking the
process on this may be very expensive if it's a medium or large volume. Consider
reading line-by-line instead, at least?

If the output here is anything other than trivial, you should also add an
optimized path where you only attach pipes and manually manage them if
"stdoutfile_header" is not None.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:87: stdoutfile_header.write(c)
You need to close this file descriptor when you're done. I recommend the "with"
/ contextmanager approach that I outlined above to avoid this sort of error.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:89: rc = process.returncode
Just use "process.returncode" in the 'print' and 'return' clauses, no need for
this indirect variable.

[nednguyen, 2017-05-19 21:21:23]

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:78: rc = 1
On 2017/05/19 20:56:18, dnj wrote:
> No point setting this here.

Done.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:81: stdoutfile_header = open(stdoutfile, 'w')
On 2017/05/19 20:56:18, dnj wrote:
> nit: do this in try/finally clause, closing on finally.
> 
> Alternatively, you can use "with" and a special contextmanager:
> 
> @contextlib.contextmanager
> def maybe_open(path, mode):
>   if not path:
>     yield None
>     return
> 
>   with open(path, mode) as fd:
>     yield fd

We no longer need this as I just assert stdoutfile is not None

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:82: process = subprocess.Popen(argv, env=env, cwd=cwd,
stdout=subprocess.PIPE,
On 2017/05/19 20:56:18, dnj wrote:
> Note that before, if the argv[0] did not exist or wasn't executable, you would
> catch that in the "except Exception" block; now, it will raise an Exception.
Is
> this OK?

That's ok as long as we have error message can point out what happened.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:84: for c in iter(lambda: process.stdout.read(1), ''):
On 2017/05/19 20:56:18, dnj wrote:
> How much data are we expecting here? Reading byte-by-byte and blocking the
> process on this may be very expensive if it's a medium or large volume.
Consider
> reading line-by-line instead, at least?
> 
> If the output here is anything other than trivial, you should also add an
> optimized path where you only attach pipes and manually manage them if
> "stdoutfile_header" is not None.

This can depend. I think it's safer to not assuming the volume is small. I
changed the code to use a polling loop for write to stdout instead.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:87: stdoutfile_header.write(c)
On 2017/05/19 20:56:18, dnj wrote:
> You need to close this file descriptor when you're done. I recommend the
"with"
> / contextmanager approach that I outlined above to avoid this sort of error.

Done.

https://codereview.chromium.org/2889153007/diff/20001/testing/scripts/common....
testing/scripts/common.py:89: rc = process.returncode
On 2017/05/19 20:56:18, dnj wrote:
> Just use "process.returncode" in the 'print' and 'return' clauses, no need for
> this indirect variable.

Done.

[nednguyen, 2017-05-19 21:23:06]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 21:23:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnj, 2017-05-19 21:27:16]

lgtm w/ comments/nits

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:76: def run_command_with_output(argv, stdoutfile,
env=None, cwd=None, ):
Nit, trailing comma.

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:81: with io.open(stdoutfile, 'wb') as writer:
Nit, you can combine this into a single block:

with io.open(...) as writer, io.open(...) as reader:

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:82: with io.open(stdoutfile, 'rb', 1) as reader:
Note: the original used "w" (text-mode); "wb" and "rb" are *probably* fine, but
double-check that nothing relied on Python's text conversion.

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:87: time.sleep(1)
Note: if the subprocess is fast and repeated (e.g., 100x 10ms surprocesses),
this will add real delay. Consider something more aggressive, like sleep(0.1).

[nednguyen, 2017-05-19 21:31:15]

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:76: def run_command_with_output(argv, stdoutfile,
env=None, cwd=None, ):
On 2017/05/19 21:27:16, dnj wrote:
> Nit, trailing comma.

Done.

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:81: with io.open(stdoutfile, 'wb') as writer:
On 2017/05/19 21:27:16, dnj wrote:
> Nit, you can combine this into a single block:
> 
> with io.open(...) as writer, io.open(...) as reader:

Done.

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:82: with io.open(stdoutfile, 'rb', 1) as reader:
On 2017/05/19 21:27:16, dnj wrote:
> Note: the original used "w" (text-mode); "wb" and "rb" are *probably* fine,
but
> double-check that nothing relied on Python's text conversion.

Oh, I just use "w" & "r" here to be sure.

https://codereview.chromium.org/2889153007/diff/40001/testing/scripts/common....
testing/scripts/common.py:87: time.sleep(1)
On 2017/05/19 21:27:16, dnj wrote:
> Note: if the subprocess is fast and repeated (e.g., 100x 10ms surprocesses),
> this will add real delay. Consider something more aggressive, like sleep(0.1).

Done.

[nednguyen, 2017-05-19 21:31:45]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2017-05-19 21:31:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/2889153007/#ps60001
(title: "Address Daniel's comments")

[commit-bot: I haz the power, 2017-05-19 21:33:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 22:54:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495229505627240,
"parent_rev": "21501560f5074b5e07c397fedc09d47a0a6c5b3f", "commit_rev":
"c3648c4f352731076b62b779597e40fed8db78e6"}

[commit-bot: I haz the power, 2017-05-19 22:55:10]

Description was changed from

==========
Update run_gtest_perf_test.py to streaming command output to stdout & log file
at the same time

BUG=723821
TBR=dpranke@chromium.org
==========

to

==========
Update run_gtest_perf_test.py to streaming command output to stdout & log file
at the same time

BUG=723821
TBR=dpranke@chromium.org

Review-Url: https://codereview.chromium.org/2889153007
Cr-Commit-Position: refs/heads/master@{#473376}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e...
==========

[commit-bot: I haz the power, 2017-05-19 22:55:11]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c3648c4f352731076b62b779597e...