Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893063003:
Include logcat created by StrictMode and test_runner_py. (Closed)
|
Created:
3 years, 7 months ago by BigBossZhiling Modified:
3 years, 7 months ago Reviewers:
jbudorick CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude logcat created by StrictMode and test_runner_py.
BUG=724528
Review-Url: https://codereview.chromium.org/2893063003
Cr-Commit-Position: refs/heads/master@{#474002}
Committed: https://chromium.googlesource.com/chromium/src/+/22a0819bb760a2a6f96a1058e488a596022545c6
Patch Set 1 #
Total comments: 8
Patch Set 2 : fixes #Patch Set 3 : create test endpoint context manager #Patch Set 4 : fixes #
Total comments: 1
Patch Set 5 : fixes #Messages
Total messages: 14 (6 generated)
Description was changed from ========== Include logcat created by StrictMode and test_runner_py. BUG=724528 ========== to ========== Include logcat created by StrictMode and test_runner_py. BUG=724528 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:54: 'StrictMode:D', 'test_runner_py:I'] nit: '%s:I' % _TAG instead of 'test_runner_py:I' https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:359: device.RunShellCommand( You'll need to move this call and the END call inside the logcat monitor context manager. It may be easiest to do so by moving the two calls into their own context manager, e.g. @contextlib.contextmanager def log_test_endpoints(): # START call try: yield finally: # END call and then using that inside the logcat monitor.
https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:54: 'StrictMode:D', 'test_runner_py:I'] On 2017/05/21 19:10:29, jbudorick wrote: > nit: > > '%s:I' % _TAG > > instead of > > 'test_runner_py:I' Done. https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:359: device.RunShellCommand( On 2017/05/21 19:10:29, jbudorick wrote: > You'll need to move this call and the END call inside the logcat monitor context > manager. It may be easiest to do so by moving the two calls into their own > context manager, e.g. > > @contextlib.contextmanager > def log_test_endpoints(): > # START call > try: > yield > finally: > # END call > > and then using that inside the logcat monitor. Do you mean logdog logcat monitor? logcat monitor is in catapult. So I assume it is in another cl?
https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:359: device.RunShellCommand( On 2017/05/22 07:07:36, BigBossZhiling wrote: > On 2017/05/21 19:10:29, jbudorick wrote: > > You'll need to move this call and the END call inside the logcat monitor > context > > manager. It may be easiest to do so by moving the two calls into their own > > context manager, e.g. > > > > @contextlib.contextmanager > > def log_test_endpoints(): > > # START call > > try: > > yield > > finally: > > # END call > > > > and then using that inside the logcat monitor. > > Do you mean logdog logcat monitor? logcat monitor is in catapult. So I assume it > is in another cl? Yeah, meant the LogdogLogcatMonitor below but was lazy about typing it. https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:373: logmon, self._test_instance.should_save_logcat): i.e., that logging needs to be done within this context manager in order to be captured in the per-test logcats.
https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:359: device.RunShellCommand( On 2017/05/22 13:14:05, jbudorick wrote: > On 2017/05/22 07:07:36, BigBossZhiling wrote: > > On 2017/05/21 19:10:29, jbudorick wrote: > > > You'll need to move this call and the END call inside the logcat monitor > > context > > > manager. It may be easiest to do so by moving the two calls into their own > > > context manager, e.g. > > > > > > @contextlib.contextmanager > > > def log_test_endpoints(): > > > # START call > > > try: > > > yield > > > finally: > > > # END call > > > > > > and then using that inside the logcat monitor. > > > > Do you mean logdog logcat monitor? logcat monitor is in catapult. So I assume > it > > is in another cl? > > Yeah, meant the LogdogLogcatMonitor below but was lazy about typing it. Done. https://codereview.chromium.org/2893063003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:373: logmon, self._test_instance.should_save_logcat): On 2017/05/22 13:14:05, jbudorick wrote: > i.e., that logging needs to be done within this context manager in order to be > captured in the per-test logcats. Done.
lgtm w/ nit https://codereview.chromium.org/2893063003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2893063003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:71: def log_test_endpoints(device, test_name): nit: _LogTestEndpoints, since this is at module scope.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2893063003/#ps80001 (title: "fixes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495560108656320,
"parent_rev": "ecc3d05879b07d9ed822b6b795872091d4e86b7b", "commit_rev":
"22a0819bb760a2a6f96a1058e488a596022545c6"}
Message was sent while issue was closed.
Description was changed from ========== Include logcat created by StrictMode and test_runner_py. BUG=724528 ========== to ========== Include logcat created by StrictMode and test_runner_py. BUG=724528 Review-Url: https://codereview.chromium.org/2893063003 Cr-Commit-Position: refs/heads/master@{#474002} Committed: https://chromium.googlesource.com/chromium/src/+/22a0819bb760a2a6f96a1058e488... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/22a0819bb760a2a6f96a1058e488... |
