Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201833002:
Added tombstones in instrumentation tests results. (Closed)
|
Created:
4 years, 4 months ago by BigBossZhiling Modified:
4 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded tombstones in instrumentation tests results.
If an instrumentation test crashes, the cl will grab the tombstones and
add the tombstones as a part of the result.
BUG=631213
Committed: https://crrev.com/d7dd97f2828447fb9af45ea119e703500b167cb8
Cr-Commit-Position: refs/heads/master@{#410929}
Patch Set 1 #
Total comments: 16
Patch Set 2 : fixes #
Total comments: 7
Patch Set 3 : Added tombstones in instrumentation tests results. #
Total comments: 4
Patch Set 4 : minor fixes #
Total comments: 12
Patch Set 5 : fixes #Patch Set 6 : fixes #
Total comments: 6
Patch Set 7 : fixes #Patch Set 8 : indentation fixes #
Total comments: 9
Patch Set 9 : fixes #
Total comments: 8
Patch Set 10 : fixes #
Total comments: 6
Patch Set 11 : address comments #
Messages
Total messages: 41 (12 generated)
Description was changed from ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ========== to ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
Description was changed from ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ========== to ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:396: self._tombstones = False Both the argument and the variable should have a slightly different name. As is, it's not clear that this is a flag -- instead, the name implies that this is storing the tombstones. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:541: def _initializeTombStonesAttributes(self, args): nit: Tombstones, not TombStones https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/test_result.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/test_result.py:34: self._tombstones = tombstones ... as opposed to this, which _is_ storing the tombstones. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:147: subprocess.check_call(['python', This isn't how we should be using tombstones. Instead, this should be importing the tombstones module and calling its methods directly. This may require shuffling tombstones.py around a bit. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:339: tombstones = subprocess.check_output(['python', Same as above. https://codereview.chromium.org/2201833002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/test_runner.p... build/android/test_runner.py:425: group.add_argument('--tombstones', dest='tombstones', Something like --store-tombstones or --capture-tombstones etc would be preferable. https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py... build/android/tombstones.py:215: def _clearAllTombstones(device): nit: _ClearAllTombstones https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py... build/android/tombstones.py:216: """Clear all tombstones in the device.. nit: only one . at the end
https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:396: self._tombstones = False On 2016/08/01 17:39:18, jbudorick wrote: > Both the argument and the variable should have a slightly different name. As is, > it's not clear that this is a flag -- instead, the name implies that this is > storing the tombstones. Done. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:541: def _initializeTombStonesAttributes(self, args): On 2016/08/01 17:39:18, jbudorick wrote: > nit: Tombstones, not TombStones Done. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/test_result.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/test_result.py:34: self._tombstones = tombstones On 2016/08/01 17:39:18, jbudorick wrote: > ... as opposed to this, which _is_ storing the tombstones. Acknowledged. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:147: subprocess.check_call(['python', On 2016/08/01 17:39:18, jbudorick wrote: > This isn't how we should be using tombstones. Instead, this should be importing > the tombstones module and calling its methods directly. This may require > shuffling tombstones.py around a bit. Done. https://codereview.chromium.org/2201833002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:339: tombstones = subprocess.check_output(['python', On 2016/08/01 17:39:18, jbudorick wrote: > Same as above. Done. https://codereview.chromium.org/2201833002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/test_runner.p... build/android/test_runner.py:425: group.add_argument('--tombstones', dest='tombstones', On 2016/08/01 17:39:18, jbudorick wrote: > Something like --store-tombstones or --capture-tombstones etc would be > preferable. Done. https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py... build/android/tombstones.py:215: def _clearAllTombstones(device): On 2016/08/01 17:39:18, jbudorick wrote: > nit: _ClearAllTombstones Done. https://codereview.chromium.org/2201833002/diff/1/build/android/tombstones.py... build/android/tombstones.py:216: """Clear all tombstones in the device.. On 2016/08/01 17:39:18, jbudorick wrote: > nit: only one . at the end Done.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:20: sys.path.append(os.path.join('..', '..', '..')) I dont think you want to add ../../.. to the system path. Probably want to add an absolute path here. Probably just add like (constants.DIR_SOURCE_ROOT, 'build', 'android') https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:178: every_tombstone: Whether to resolve every tombstone. Maybe rename to resolve_all_tombstones. Its awkward having a list called all_tombstones and a boolean called every_tombstone. They could be interchangeable. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:179: stack: Stack. Probably at least write the datatype for the stack. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:225: logging.warning('No tombstones.') Maybe slightly more descriptive warning. Like "No tombstones to clear" or something. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:285: ResolveAllTombstones(device, args.all_tombstones, This indentation looks off.
https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:20: sys.path.append(os.path.join('..', '..', '..')) On 2016/08/01 22:44:30, mikecase wrote: > I dont think you want to add ../../.. to the system path. Probably want to add > an absolute path here. > > Probably just add like (constants.DIR_SOURCE_ROOT, 'build', 'android') Do you mean I should get the absolute path here? And then remove the last several '/***'? Or is there a constants variable? https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:178: every_tombstone: Whether to resolve every tombstone. On 2016/08/01 22:44:30, mikecase wrote: > Maybe rename to resolve_all_tombstones. > > Its awkward having a list called all_tombstones and a boolean called > every_tombstone. They could be interchangeable. Done. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:179: stack: Stack. On 2016/08/01 22:44:30, mikecase wrote: > Probably at least write the datatype for the stack. I think it is boolean. But I don't know what this boolean stand for. It was just args.stack. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:225: logging.warning('No tombstones.') On 2016/08/01 22:44:30, mikecase wrote: > Maybe slightly more descriptive warning. Like "No tombstones to clear" or > something. Done. https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:285: ResolveAllTombstones(device, args.all_tombstones, On 2016/08/01 22:44:30, mikecase wrote: > This indentation looks off. Done.
https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/tombstone... build/android/tombstones.py:178: every_tombstone: Whether to resolve every tombstone. On 2016/08/01 22:44:30, mikecase wrote: > Maybe rename to resolve_all_tombstones. > > Its awkward having a list called all_tombstones and a boolean called > every_tombstone. They could be interchangeable. Done.
https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:20: sys.path.append(os.path.join('..', '..', '..')) On 2016/08/01 at 23:04:35, BigBossZhiling wrote: > On 2016/08/01 22:44:30, mikecase wrote: > > I dont think you want to add ../../.. to the system path. Probably want to add > > an absolute path here. > > > > Probably just add like (constants.DIR_SOURCE_ROOT, 'build', 'android') > > Do you mean I should get the absolute path here? And then remove the last several '/***'? Or is there a constants variable? This doesnt work I dont think. Im 80% sure this path is just relative to where you run this script from. Just import pylib.constants and use the CHROMIUM_SRC variable from there. Or add __file__ to the beginning of the path you have. https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... build/android/tombstones.py:179: every_tombstone: Whether to resolve every tombstone. update name https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... build/android/tombstones.py:180: stack: Stack. Same comment as before. At least add the datatype for the stack or more info. Just write something like "Whether to include symbols for stack data." or something.
https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... build/android/tombstones.py:179: every_tombstone: Whether to resolve every tombstone. On 2016/08/02 00:48:30, mikecase wrote: > update name Done. https://codereview.chromium.org/2201833002/diff/40001/build/android/tombstone... build/android/tombstones.py:180: stack: Stack. On 2016/08/02 00:48:31, mikecase wrote: > Same comment as before. > > At least add the datatype for the stack or more info. > > Just write something like "Whether to include symbols for stack data." or > something. Done.
just some nits left https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:541: def _initializeStoreTombstonesAttributes(self, args): nit: this should be named _initializeTombstonesAttributes or like something more general even like _initializeLoggingAttributes or something? https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/test_result.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/test_result.py:33: def set_tombstones(self, tombstones): nit: SetTombstones https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:342: tombstones.ResolveAllTombstones(device, True, False, True) nit: I think some people thinks its overly verbose, but I prefer tombstones.ResolveAllTombstones( device, resolve_all_tombstones=True, stack=False, wipe_tombstones=True) https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:179: resolve_all_tombstone: Whether to resolve every tombstone. nit: s/resolve_all_tombstone/resolve_all_tombstones https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:180: stack: Whether to include symbols for stack data.. nit: remove double period.. Probably s/stack/include_stack_symbols https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:232: jobs=4): Add documentation. For example, I dont have any idea what jobs=4 does.
https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:541: def _initializeStoreTombstonesAttributes(self, args): On 2016/08/02 17:27:29, mikecase wrote: > nit: this should be named _initializeTombstonesAttributes or like something more > general even like _initializeLoggingAttributes or something? > Done. https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/test_result.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/test_result.py:33: def set_tombstones(self, tombstones): On 2016/08/02 17:27:29, mikecase wrote: > nit: SetTombstones Done. https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:342: tombstones.ResolveAllTombstones(device, True, False, True) On 2016/08/02 17:27:29, mikecase wrote: > nit: I think some people thinks its overly verbose, but I prefer > > tombstones.ResolveAllTombstones( > device, > resolve_all_tombstones=True, > stack=False, > wipe_tombstones=True) Done. https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:179: resolve_all_tombstone: Whether to resolve every tombstone. On 2016/08/02 17:27:29, mikecase wrote: > nit: s/resolve_all_tombstone/resolve_all_tombstones Done. https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:180: stack: Whether to include symbols for stack data.. On 2016/08/02 17:27:29, mikecase wrote: > nit: remove double period.. > > Probably s/stack/include_stack_symbols Done. https://codereview.chromium.org/2201833002/diff/60001/build/android/tombstone... build/android/tombstones.py:232: jobs=4): On 2016/08/02 17:27:29, mikecase wrote: > Add documentation. For example, I dont have any idea what jobs=4 does. Done.
This lgtm with some questions and a nit. https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:152: tombstones.ClearAllTombstones(dev) There is a recipe step on our infrastructure that collects all the tombstones at the end of the test runs and lists them out. Will this break that script? I suppose as long as store_tombstones defaults to False to begin that shouldnt be a problem. https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:343: device, nit: wrong indent. Need 4 spaces. https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:347: result.SetTombstones(tombstones) So, Im not too sure what tombstones really are to be honest. Do you know if they are generated by the death tests?
https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:152: tombstones.ClearAllTombstones(dev) On 2016/08/04 01:12:05, mikecase wrote: > There is a recipe step on our infrastructure that collects all the tombstones at > the end of the test runs and lists them out. Will this break that script? I > suppose as long as store_tombstones defaults to False to begin that shouldnt be > a problem. Acknowledged. https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:343: device, On 2016/08/04 01:12:05, mikecase wrote: > nit: wrong indent. Need 4 spaces. Done. https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:347: result.SetTombstones(tombstones) On 2016/08/04 01:12:05, mikecase wrote: > So, Im not too sure what tombstones really are to be honest. Do you know if they > are generated by the death tests? What are death tests? I think if a test crashes, then a tombstone will be created. Passing and Failing testcases do not usually create tombstones. I also sent out an email containing a sample tombstone.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2201833002/#ps140001 (title: "indentation fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/04 23:20:28, BigBossZhiling wrote: > https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... > File build/android/pylib/local/device/local_device_instrumentation_test_run.py > (right): > > https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:152: > tombstones.ClearAllTombstones(dev) > On 2016/08/04 01:12:05, mikecase wrote: > > There is a recipe step on our infrastructure that collects all the tombstones > at > > the end of the test runs and lists them out. Will this break that script? I > > suppose as long as store_tombstones defaults to False to begin that shouldnt > be > > a problem. > > Acknowledged. > > https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:343: > device, > On 2016/08/04 01:12:05, mikecase wrote: > > nit: wrong indent. Need 4 spaces. > > Done. > > https://codereview.chromium.org/2201833002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_instrumentation_test_run.py:347: > result.SetTombstones(tombstones) > On 2016/08/04 01:12:05, mikecase wrote: > > So, Im not too sure what tombstones really are to be honest. Do you know if > they > > are generated by the death tests? > > What are death tests? I think if a test crashes, then a tombstone will be > created. Passing and Failing testcases do not usually create tombstones. I also > sent out an email containing a sample tombstone. A death test is a native test that asserts that some call causes the process to die. https://codesearch.chromium.org/search/?q=(ASSERT%7CEXPECT)_DEATH+case:yes+-f...
The CQ bit was unchecked by jbudorick@chromium.org
On 2016/08/04 23:34:11, jbudorick wrote: > The CQ bit was unchecked by mailto:jbudorick@chromium.org Reviewing, going to have comments that should be fixed before landing.
https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:21: sys.path.append(os.path.join(host_paths.DIR_SOURCE_ROOT, 'build', 'android')) This should already be in the path. In the event that it isn't, you should extract some of the tombstones logic into a module within pylib.utils. https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:347: result.SetTombstones(tombstones) You're passing the module here. https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... build/android/tombstones.py:153: def _ResolveTombstones(jobs, tombstones): This doesn't return the resolved tombstones. https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... build/android/tombstones.py:233: wipe_tombstones, jobs=4): nit: indentation is off here. https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... build/android/tombstones.py:243: return _ResolveTombstones(jobs, This returns what _ResolveTombstones returns, but _ResolveTombstones doesn't return anything.
https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:21: sys.path.append(os.path.join(host_paths.DIR_SOURCE_ROOT, 'build', 'android')) On 2016/08/04 23:54:18, jbudorick wrote: > This should already be in the path. In the event that it isn't, you should > extract some of the tombstones logic into a module within pylib.utils. Done. https://codereview.chromium.org/2201833002/diff/140001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:347: result.SetTombstones(tombstones) On 2016/08/04 23:54:18, jbudorick wrote: > You're passing the module here. Done. https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... build/android/tombstones.py:233: wipe_tombstones, jobs=4): On 2016/08/04 23:54:18, jbudorick wrote: > nit: indentation is off here. Done. https://codereview.chromium.org/2201833002/diff/140001/build/android/tombston... build/android/tombstones.py:243: return _ResolveTombstones(jobs, On 2016/08/04 23:54:18, jbudorick wrote: > This returns what _ResolveTombstones returns, but _ResolveTombstones doesn't > return anything. Done.
https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:10: import tombstones nit: This shouldn't be with the system imports. Put it down w/ devil and pylib. https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:21: ? https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... build/android/tombstones.py:170: resolved_tombstones.append('\n'.join(tombstone)) nit: resolved_tombstones.extend(tombstone) https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... build/android/tombstones.py:172: logging.info(line) This logging should move down into main().
https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:10: import tombstones On 2016/08/08 17:55:25, jbudorick wrote: > nit: This shouldn't be with the system imports. Put it down w/ devil and pylib. Done. https://codereview.chromium.org/2201833002/diff/160001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:21: On 2016/08/08 17:55:25, jbudorick wrote: > ? Done. https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... build/android/tombstones.py:170: resolved_tombstones.append('\n'.join(tombstone)) On 2016/08/08 17:55:25, jbudorick wrote: > nit: resolved_tombstones.extend(tombstone) Done. https://codereview.chromium.org/2201833002/diff/160001/build/android/tombston... build/android/tombstones.py:172: logging.info(line) On 2016/08/08 17:55:25, jbudorick wrote: > This logging should move down into main(). Done.
https://codereview.chromium.org/2201833002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:337: resolved_tombstones = tombstones.ResolveTombstones( This will need to be updated to handle a list of lines rather than a single, multiline string. https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... build/android/tombstones.py:171: return '\n'.join(resolved_tombstones) This should return the list of lines, not the joined string. https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... build/android/tombstones.py:300: logging.info(resolved_tombstones) w/ ResolveTombstones returning a list, you can loop through resolved_tombstones and log each line.
https://codereview.chromium.org/2201833002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2201833002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:337: resolved_tombstones = tombstones.ResolveTombstones( On 2016/08/09 22:28:47, jbudorick wrote: > This will need to be updated to handle a list of lines rather than a single, > multiline string. Done. https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... build/android/tombstones.py:171: return '\n'.join(resolved_tombstones) On 2016/08/09 22:28:47, jbudorick wrote: > This should return the list of lines, not the joined string. Done. https://codereview.chromium.org/2201833002/diff/180001/build/android/tombston... build/android/tombstones.py:300: logging.info(resolved_tombstones) On 2016/08/09 22:28:47, jbudorick wrote: > w/ ResolveTombstones returning a list, you can loop through resolved_tombstones > and log each line. Done.
lgtm I'm a bit concerned about this when we start trying to deploy it on swarming bots, but we'll address that in a later CL.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2201833002/#ps200001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ========== to ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 ========== to ========== Added tombstones in instrumentation tests results. If an instrumentation test crashes, the cl will grab the tombstones and add the tombstones as a part of the result. BUG=631213 Committed: https://crrev.com/d7dd97f2828447fb9af45ea119e703500b167cb8 Cr-Commit-Position: refs/heads/master@{#410929} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d7dd97f2828447fb9af45ea119e703500b167cb8 Cr-Commit-Position: refs/heads/master@{#410929}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2231693002/ by kjellander@chromium.org. The reason for reverting is: This breaks the stack_tool_for_tombstones step for many bots in https://build.chromium.org/p/chromium.android, like this: W 0.547s Main No tombstones. Traceback (most recent call last): File "/b/c/b/Android_Webview_L__dbg_/src/build/android/tombstones.py", line 304, in <module> sys.exit(main()) File "/b/c/b/Android_Webview_L__dbg_/src/build/android/tombstones.py", line 300, in main W 0.547s Main No tombstones to resolve. for line in resolved_tombstones: TypeError: 'NoneType' object is not iterable Please make sure to have proper tests in place before relanding.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
