Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451523002:
Insert logcat as part of test result for android instrumentation tests. (Closed)
|
Created:
4 years, 1 month ago by BigBossZhiling Modified:
4 years ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInsert logcat as part of test result for android instrumentation tests.
In this cl, we are trying to add the logcat of each instrumentation
test into the corresponding test result object, so that when we are
checking the result of each test case, we would be able to see the
corresponding logcat for that test case at the same time.
BUG=631213
Committed: https://crrev.com/db14e1adbd87e01676f5915881571552e384a8d3
Cr-Commit-Position: refs/heads/master@{#435041}
Patch Set 1 #
Total comments: 8
Patch Set 2 : remove unnecssary changes/ fixes #
Total comments: 1
Patch Set 3 : give logcat a value in case there is exception and name error happens #Patch Set 4 : using namedtemporaryfile #Patch Set 5 : using logdog #
Total comments: 15
Patch Set 6 : using logdog and write to logdog as it happens #Patch Set 7 : added the newly added file #
Total comments: 14
Patch Set 8 : fixes #Patch Set 9 : change from logcat snippet to logcat url #
Total comments: 14
Patch Set 10 : removed all unnecessary parts of logcat monitor #
Total comments: 6
Patch Set 11 : fixes #Patch Set 12 : fixes #
Messages
Total messages: 37 (8 generated)
Description was changed from ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG= ========== to ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG= ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
Description was changed from ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG= ========== to ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG=631213 ==========
https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:91: result_code, result_bundle, statuses, start_ms, duration_ms, logcat=None): This shouldn't be done in this function. Instead, you should set the logcat after GenerateTestResults returns. https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:270: for logstring in logmon.FindAll(r'.*'): There are a couple of better options for dumping the entire logcat: - create a logcat monitor w/ a temporary file, then read that file after the logcat monitor context manager goes out of scope. - add a new function to logcat monitor that returns the entire recorded logcat up to that point. I think I prefer the former to the latter. https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/results... File build/android/pylib/results/json_results.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/results... build/android/pylib/results/json_results.py:105: 'logcat': r.GetLogcat() or '', nit: logcat_snippet https://codereview.chromium.org/2451523002/diff/1/build/android/test_runner.p... File build/android/test_runner.pydeps (right): https://codereview.chromium.org/2451523002/diff/1/build/android/test_runner.p... build/android/test_runner.pydeps:3: ../../third_party/appurify-python/src/appurify/__init__.py These shouldn't be here...
https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:91: result_code, result_bundle, statuses, start_ms, duration_ms, logcat=None): On 2016/10/25 02:17:35, jbudorick wrote: > This shouldn't be done in this function. Instead, you should set the logcat > after GenerateTestResults returns. Done. https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:270: for logstring in logmon.FindAll(r'.*'): On 2016/10/25 02:17:35, jbudorick wrote: > There are a couple of better options for dumping the entire logcat: > - create a logcat monitor w/ a temporary file, then read that file after the > logcat monitor context manager goes out of scope. > - add a new function to logcat monitor that returns the entire recorded logcat > up to that point. > > I think I prefer the former to the latter. Done. https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/results... File build/android/pylib/results/json_results.py (right): https://codereview.chromium.org/2451523002/diff/1/build/android/pylib/results... build/android/pylib/results/json_results.py:105: 'logcat': r.GetLogcat() or '', On 2016/10/25 02:17:35, jbudorick wrote: > nit: logcat_snippet Done. https://codereview.chromium.org/2451523002/diff/1/build/android/test_runner.p... File build/android/test_runner.pydeps (right): https://codereview.chromium.org/2451523002/diff/1/build/android/test_runner.p... build/android/test_runner.pydeps:3: ../../third_party/appurify-python/src/appurify/__init__.py On 2016/10/25 02:17:35, jbudorick wrote: > These shouldn't be here... Acknowledged.
https://codereview.chromium.org/2451523002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:266: output_file = '/tmp/logcat/%s' % test_name Use tempfile.NamedTemporaryFile as a context manager instead of doing this: https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile
This looks fine, but I'd like to see how this affects instrumentation test performance on the trybots. I've kicked a few runs of linux_android_rel_ng and android_n5x_swarming_rel for that reason.
On 2016/10/25 22:46:26, jbudorick wrote: > This looks fine, but I'd like to see how this affects instrumentation test > performance on the trybots. I've kicked a few runs of linux_android_rel_ng and > android_n5x_swarming_rel for that reason. Right. It is possible that writing logcat to a temporary file and read it for each test case may be time consuming. If that is the case, I can just add a method in logcatmonitor to return the logcat.
On 2016/10/25 22:46:26, jbudorick wrote: > This looks fine, but I'd like to see how this affects instrumentation test > performance on the trybots. I've kicked a few runs of linux_android_rel_ng and > android_n5x_swarming_rel for that reason. But if it isn't taking too much time, I think the code now is good to go then.
On 2016/10/25 22:49:46, BigBossZhiling wrote: > On 2016/10/25 22:46:26, jbudorick wrote: > > This looks fine, but I'd like to see how this affects instrumentation test > > performance on the trybots. I've kicked a few runs of linux_android_rel_ng and > > android_n5x_swarming_rel for that reason. > > But if it isn't taking too much time, I think the code now is good to go then. Yeah, I agree.
** Presubmit Warnings ** Pylint (135 files using ['--disable=cyclic-import'] on 48 cores) (2.51s) failed ************* Module pylib.logcat_monitor_using_logdog F: 15, 0: Unable to import 'libs.logdog' (import-error) Have this issue when I do cl upload. But I don't run into any import error when I run the code.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... File build/android/pylib/logcat_monitor_using_logdog.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:12: sys.path) Im guessing that is because you are adding this to the path here. Just add a #pylint ignore comment.
https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/bas... File build/android/pylib/base/base_test_result.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/bas... build/android/pylib/base/base_test_result.py:99: def SetLogcat(self, logcat): Should these two be *LogcatURL now? https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: with logcat_monitor_using_logdog.LogcatMonitorUsingLogdog(device.adb, This should totally be something like "LogcatdogMonitor" :P (I do think that this name should not include "Using," though I'm not sure what it should be.) https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... File build/android/pylib/logcat_monitor_using_logdog.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. nit: put this in a new pylib/android directory, plz. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:11: sys.path = ([os.path.join(CHROMIUM_SRC_DIR, 'tools', 'swarming_client')] + This shold be: from pylib import constants sys.path.append(os.path.abspath(os.path.join( constants.DIR_SOURCE_ROOT, 'tools', 'swarming_client'))) from libs.logdog import bootstrap https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:22: def GetLogcatURL(self): Line breaks between methods + docstrings for public methods & for the class, please. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:24: def Stop(self): Can we implement this s.t. we write to the logdog stream on the recording thread as it's happening rather than writing it at all to the stream at the end? https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/res... File build/android/pylib/results/json_results.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/res... build/android/pylib/results/json_results.py:105: 'logcat_snippet': r.GetLogcat() or '', This should be something like 'logcat_url' now, right?
https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/bas... File build/android/pylib/base/base_test_result.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/bas... build/android/pylib/base/base_test_result.py:99: def SetLogcat(self, logcat): On 2016/11/18 02:31:42, jbudorick wrote: > Should these two be *LogcatURL now? Done. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:268: with logcat_monitor_using_logdog.LogcatMonitorUsingLogdog(device.adb, On 2016/11/18 02:31:42, jbudorick wrote: > This should totally be something like "LogcatdogMonitor" :P > > (I do think that this name should not include "Using," though I'm not sure what > it should be.) Done. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... File build/android/pylib/logcat_monitor_using_logdog.py (right): https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/18 02:31:42, jbudorick wrote: > nit: put this in a new pylib/android directory, plz. Done. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:11: sys.path = ([os.path.join(CHROMIUM_SRC_DIR, 'tools', 'swarming_client')] + On 2016/11/18 02:31:42, jbudorick wrote: > This shold be: > > from pylib import constants > > sys.path.append(os.path.abspath(os.path.join( > constants.DIR_SOURCE_ROOT, 'tools', 'swarming_client'))) > > from libs.logdog import bootstrap Done. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:12: sys.path) On 2016/11/05 01:27:53, mikecase wrote: > Im guessing that is because you are adding this to the path here. Just add a > #pylint ignore comment. Acknowledged. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:22: def GetLogcatURL(self): On 2016/11/18 02:31:42, jbudorick wrote: > Line breaks between methods + docstrings for public methods & for the class, > please. Done. https://codereview.chromium.org/2451523002/diff/80001/build/android/pylib/log... build/android/pylib/logcat_monitor_using_logdog.py:24: def Stop(self): On 2016/11/18 02:31:42, jbudorick wrote: > Can we implement this s.t. we write to the logdog stream on the recording thread > as it's happening rather than writing it at all to the stream at the end? Done.
I have done gclient sync. Thus if you do the patch comparison, you will actually see changes brought in by gclient sync, which are done by other people in that file.
https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... File build/android/pylib/android/logcat_monitor_with_logdog.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:17: class LogcatMonitorWithLogdog(logcat_monitor.LogcatMonitor): Rename this LogdogLogcatMonitor. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:19: The logdog stream client will return a url, where contains the logcat. nit: docstrings should either be """Summary line.""" or """Summary line. Longer description. """ w/ a blank line between the summary and the description. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:61: self._record_file.write(data + '\n') Why should this continue writing to file? https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:25: constants.DIR_SOURCE_ROOT, 'build', 'android', 'pylib', 'android'))) You shouldn't need to do this. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:26: import logcat_monitor_with_logdog You should be able to import this as from pylib.android import logdog_logcat_monitor https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:257: with logcat_monitor_with_logdog.LogcatMonitorWithLogdog( I'm concerned about always doing this, particularly for local development & w/ that logging.exception call you've got in LogdogLogcatMonitor.__init__. I think that's liable to confuse people. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/re... File build/android/pylib/results/json_results.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/re... build/android/pylib/results/json_results.py:105: 'logcat_snippet': r.GetLogcatUrl() or '', nit: I think this should be 'logcat_url'
https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... File build/android/pylib/android/logcat_monitor_with_logdog.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:17: class LogcatMonitorWithLogdog(logcat_monitor.LogcatMonitor): On 2016/11/22 14:40:14, jbudorick wrote: > Rename this LogdogLogcatMonitor. Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:19: The logdog stream client will return a url, where contains the logcat. On 2016/11/22 14:40:14, jbudorick wrote: > nit: docstrings should either be > > """Summary line.""" > > or > > """Summary line. > > Longer description. > """ > > w/ a blank line between the summary and the description. Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/an... build/android/pylib/android/logcat_monitor_with_logdog.py:61: self._record_file.write(data + '\n') On 2016/11/22 14:40:14, jbudorick wrote: > Why should this continue writing to file? Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:25: constants.DIR_SOURCE_ROOT, 'build', 'android', 'pylib', 'android'))) On 2016/11/22 14:40:15, jbudorick wrote: > You shouldn't need to do this. Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:26: import logcat_monitor_with_logdog On 2016/11/22 14:40:14, jbudorick wrote: > You should be able to import this as > > from pylib.android import logdog_logcat_monitor Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:257: with logcat_monitor_with_logdog.LogcatMonitorWithLogdog( On 2016/11/22 14:40:14, jbudorick wrote: > I'm concerned about always doing this, particularly for local development & w/ > that logging.exception call you've got in LogdogLogcatMonitor.__init__. I think > that's liable to confuse people. Done. https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/re... File build/android/pylib/results/json_results.py (right): https://codereview.chromium.org/2451523002/diff/120001/build/android/pylib/re... build/android/pylib/results/json_results.py:105: 'logcat_snippet': r.GetLogcatUrl() or '', On 2016/11/22 14:40:15, jbudorick wrote: > nit: I think this should be 'logcat_url' Done.
Getting close. Just a few things to work out w.r.t. record_file. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... File build/android/pylib/android/logdog_logcat_monitor.py (right): https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:12: sys.path.append(os.path.abspath(os.path.join( nit: +1 blank line here https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:17: class LogdogLogcatMonitor(logcat_monitor.LogcatMonitor): This is still doing a lot with _record_file that it doesn't need to. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:23: self.logcat_url = '' These should be prefixed w/ _ (e.g. self._logcat_url) https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:45: super(LogdogLogcatMonitor, self).Stop() LogcatMonitor.Stop calls _StopRecording and does a bunch of file manipulation. This class should only need to call _StopRecording. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:55: def record_to_file(): nit: record_to_stream? https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:60: with self._record_file_lock: I don't think this should have to grab the _record_file_lock... https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:601: self._json_results_file = args.json_results_file nit: I would store this as self._should_save_logcat or something, since we don't care what the json_results_file is, just whether or not it exists for the purposes of saving the logcation.
Please review. I have removed every part of the unnecessary logcat monitor. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... File build/android/pylib/android/logdog_logcat_monitor.py (right): https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:12: sys.path.append(os.path.abspath(os.path.join( On 2016/11/23 16:25:20, jbudorick wrote: > nit: +1 blank line here Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:17: class LogdogLogcatMonitor(logcat_monitor.LogcatMonitor): On 2016/11/23 16:25:21, jbudorick wrote: > This is still doing a lot with _record_file that it doesn't need to. Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:23: self.logcat_url = '' On 2016/11/23 16:25:21, jbudorick wrote: > These should be prefixed w/ _ (e.g. self._logcat_url) Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:45: super(LogdogLogcatMonitor, self).Stop() On 2016/11/23 16:25:20, jbudorick wrote: > LogcatMonitor.Stop calls _StopRecording and does a bunch of file manipulation. > This class should only need to call _StopRecording. Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:55: def record_to_file(): On 2016/11/23 16:25:20, jbudorick wrote: > nit: record_to_stream? Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/an... build/android/pylib/android/logdog_logcat_monitor.py:60: with self._record_file_lock: On 2016/11/23 16:25:21, jbudorick wrote: > I don't think this should have to grab the _record_file_lock... Done. https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/160001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:601: self._json_results_file = args.json_results_file On 2016/11/23 16:25:21, jbudorick wrote: > nit: I would store this as self._should_save_logcat or something, since we don't > care what the json_results_file is, just whether or not it exists for the > purposes of saving the logcation. Done.
Also there are these two warnings: ************* Module pylib.android.logdog_logcat_monitor F: 17, 0: Unable to import 'libs.logdog' (import-error) W: 23, 2: __init__ method from base class 'LogcatMonitor' is not called (super-init-not-called) I think they seem to be reasonable to exist?
On 2016/11/29 00:42:15, BigBossZhiling wrote: > Also there are these two warnings: > ************* Module pylib.android.logdog_logcat_monitor > F: 17, 0: Unable to import 'libs.logdog' (import-error) This one's reasonable; you can disable it w/ # pylint: disable=import-error at the end of that line. > W: 23, 2: __init__ method from base class 'LogcatMonitor' is not called > (super-init-not-called) This one is an interesting case. I think you ought to be calling the superclass's __init__ method even though that winds up creating a few variables you don't need. You should be able to remove some of the logic duplicated between the two from LogdogLogcatMonitor.__init__. > > I think they seem to be reasonable to exist?
https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:445: self._json_results_file = None self._should_save_logcat https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:446: self._initializeJsonResultsFileAttributes(args) nit: this seems overly specific; maybe _initializeResultsAttributes or _initializeLogAttributes? https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:257: logcat_url = logmon.GetLogcatURL() You need to do this outside the scope of the with block. Inside it, we haven't called __exit__ or Stop and thus haven't set the logcat url yet.
quick fixes https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:445: self._json_results_file = None On 2016/11/29 00:50:59, jbudorick wrote: > self._should_save_logcat Done. https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:446: self._initializeJsonResultsFileAttributes(args) On 2016/11/29 00:50:59, jbudorick wrote: > nit: this seems overly specific; maybe _initializeResultsAttributes or > _initializeLogAttributes? Done. https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2451523002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:257: logcat_url = logmon.GetLogcatURL() On 2016/11/29 00:50:59, jbudorick wrote: > You need to do this outside the scope of the with block. Inside it, we haven't > called __exit__ or Stop and thus haven't set the logcat url yet. Done.
On 2016/11/29 00:45:37, jbudorick wrote: > On 2016/11/29 00:42:15, BigBossZhiling wrote: > > Also there are these two warnings: > > ************* Module pylib.android.logdog_logcat_monitor > > F: 17, 0: Unable to import 'libs.logdog' (import-error) > > This one's reasonable; you can disable it w/ # pylint: disable=import-error at > the end of that line. > > > W: 23, 2: __init__ method from base class 'LogcatMonitor' is not called > > (super-init-not-called) > > This one is an interesting case. I think you ought to be calling the > superclass's __init__ method even though that winds up creating a few variables > you don't need. You should be able to remove some of the logic duplicated > between the two from LogdogLogcatMonitor.__init__. > > > > > I think they seem to be reasonable to exist? What about these?
done. Cleared the warning errors.
lgtm
The CQ bit was checked by hzl@google.com
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": 220001, "attempt_start_ts": 1480440639235050,
"parent_rev": "afe30b218867b458c1a915a109631958cb9e9cd2", "commit_rev":
"3e1b42e9cfedbb3684dee0af4d94b3f433932275"}
Message was sent while issue was closed.
Description was changed from ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG=631213 ========== to ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG=631213 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG=631213 ========== to ========== Insert logcat as part of test result for android instrumentation tests. In this cl, we are trying to add the logcat of each instrumentation test into the corresponding test result object, so that when we are checking the result of each test case, we would be able to see the corresponding logcat for that test case at the same time. BUG=631213 Committed: https://crrev.com/db14e1adbd87e01676f5915881571552e384a8d3 Cr-Commit-Position: refs/heads/master@{#435041} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/db14e1adbd87e01676f5915881571552e384a8d3 Cr-Commit-Position: refs/heads/master@{#435041}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2539953002/ by jbudorick@chromium.org. The reason for reverting is: breaking on chromium.android: https://build.chromium.org/p/chromium.android/builders/Marshmallow%20Tablet%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
