Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2012323002:
[Android] Implement perf tests to platform mode. (Closed)
|
Created:
4 years, 6 months ago by rnephew (Reviews Here) Modified:
4 years, 5 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. |
Description[Android] Implement perf tests to platform mode.
BUG=615157, 590229
Committed: https://crrev.com/8db92cc2828e9757b91662a1ed7057f17f8024ed
Cr-Commit-Position: refs/heads/master@{#404978}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : now tracking origin/master #Patch Set 4 : fix reabse issue in test_runner.py #Patch Set 5 : add retry logic and some clean up #
Total comments: 40
Patch Set 6 : bug fixes and shuffling #
Total comments: 25
Patch Set 7 : address comments in ps5 #
Total comments: 5
Patch Set 8 : #Patch Set 9 : Unify RunTestsInPlatformMode and move adbd restart #
Total comments: 61
Patch Set 10 : #Patch Set 11 : do not run as default and rebase #
Total comments: 56
Patch Set 12 : Johns comments #Patch Set 13 : Created subclasses for perf test run #
Total comments: 45
Patch Set 14 : [Android] Implement perf tests to platform mode. #
Total comments: 12
Patch Set 15 : [Android] Implement perf tests to platform mode. #
Total comments: 28
Patch Set 16 : [Android] Implement perf tests to platform mode. #
Total comments: 25
Patch Set 17 : cases comments and rebase #Patch Set 18 : johns comments #Messages
Total messages: 50 (14 generated)
https://codereview.chromium.org/2012323002/diff/40001/build/android/test_runn... File build/android/test_runner.py (left): https://codereview.chromium.org/2012323002/diff/40001/build/android/test_runn... build/android/test_runner.py:856: # TODO(jbudorick): Rewrite results handling. Note: I did not do these changes, showed up after the rebase.
Patchset #3 (id:40001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
rnephew@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, yolandyan@chromium.org
Should mostly be ready for review. I might shuffle things around and make small edits; but the majority of it is in place.
https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... File build/android/pylib/base/test_instance_factory.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... build/android/pylib/base/test_instance_factory.py:22: elif args.command == 'perf': suuuuuuuper nit: alphabetize maybe https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:36: return (local_device_perf_test_run.LocalDevicePerfTestRun(env, nit: the outer parenthesis arent necessary. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. nit 2016 https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:37: # How often to print the heartbeat on flush(). nit: add "in <time_unit>" (Im guessing it is seconds) to the end of comment. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:60: print '--single-step output length %d' % self._len Since you are doing this on a separate thread I would probably use the logging module which is thread-safe. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:71: See: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... This comment is probably going to get out of date since the line number could change. You "could" point towards a specific revision instead of master, but I think that is also unwise. Idk, I would probably just remove think link. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:77: status, output = cmd_helper.GetCmdStatusAndOutput( you will have to set cwd=CHROMIUM_SRC_DIR or something for this. Otherwise, if someone runs the test_runner from a different directory, they might not even be in a git repo https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:108: self._retries = retries nit: probably just alphabetize all of the things OR put then in the order they are listed out as args. I think either of those makes sense. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:128: logging.exception('Exception when resetting ports.') I would say this is fine except you also have RestartAdbd() in the try block. This could output a very misleading error message if that is the things that raises the exception. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:174: else self._tests[test].get('timeout', 3600)) probably factor out this 3600 somewhere and make it a _CONSTANT https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:184: cwd = os.path.abspath(os.path.join(host_paths.DIR_SOURCE_ROOT, os.pardir)) Im not a fan of things like this. Probably would be nice to work to remove the need for this. So some tests just have to be run from a certain directory? https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:213: self._device.WaitUntilFullyBooted(timeout=120) We should probably have some shared GetDeviceBootTimeout function in constants.py that looks at device type and Android version and just returns a good timeout. At least, for the time being, would be nice to factor the 120 out to the top of the module. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:268: result['output'] = previous['output'] + result['output'] Do this save files ever get cleared? It looks like they will just keep on growing forever. Is that a problem? https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. nit 2016 https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... build/android/test_runner.py:712: return test_run.RunTests() Not sure I 100% sure I get what this does. https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... build/android/test_runner.py:857: 'perf', super nit: alphabetize
PS5 comments, starting to review PS6 https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... File build/android/pylib/base/test_instance_factory.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... build/android/pylib/base/test_instance_factory.py:22: elif args.command == 'perf': nit: alphabetize https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:27: self._STR = 'Local Device Perf Test Instance: ' Nothing in here should be specific to a local device. Logic that is belongs in local_device_perf_test_run.py. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:32: self.steps = args.steps 1) Why is so much of this public? It really shouldn't be. Read-only properties would be vastly preferable. 2) alphabetize https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:72: nit: only one line between member functions
https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... File build/android/pylib/base/test_instance_factory.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... build/android/pylib/base/test_instance_factory.py:22: elif args.command == 'perf': On 2016/06/01 17:40:27, mikecase wrote: > suuuuuuuper nit: alphabetize maybe Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:36: return (local_device_perf_test_run.LocalDevicePerfTestRun(env, On 2016/06/01 17:40:27, mikecase wrote: > nit: the outer parenthesis arent necessary. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/01 17:40:27, mikecase wrote: > nit 2016 Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:37: # How often to print the heartbeat on flush(). On 2016/06/01 17:40:28, mikecase wrote: > nit: add "in <time_unit>" (Im guessing it is seconds) to the end of comment. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:60: print '--single-step output length %d' % self._len On 2016/06/01 17:40:28, mikecase wrote: > Since you are doing this on a separate thread I would probably use the logging > module which is thread-safe. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:77: status, output = cmd_helper.GetCmdStatusAndOutput( On 2016/06/01 17:40:28, mikecase wrote: > you will have to set cwd=CHROMIUM_SRC_DIR or something for this. Otherwise, if > someone runs the test_runner from a different directory, they might not even be > in a git repo It already does, the host_paths.DIR_SOURCE_ROOT part is cwd=. I explicitly state it now though, since it is an optional thing. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:108: self._retries = retries On 2016/06/01 17:40:28, mikecase wrote: > nit: probably just alphabetize all of the things OR put then in the order they > are listed out as args. I think either of those makes sense. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:128: logging.exception('Exception when resetting ports.') On 2016/06/01 17:40:27, mikecase wrote: > I would say this is fine except you also have RestartAdbd() in the try block. > This could output a very misleading error message if that is the things that > raises the exception. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:174: else self._tests[test].get('timeout', 3600)) On 2016/06/01 17:40:27, mikecase wrote: > probably factor out this 3600 somewhere and make it a _CONSTANT Switched it to use the class varialbe _timeout which is also used by the watchdog timer. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:184: cwd = os.path.abspath(os.path.join(host_paths.DIR_SOURCE_ROOT, os.pardir)) On 2016/06/01 17:40:28, mikecase wrote: > Im not a fan of things like this. Probably would be nice to work to remove the > need for this. So some tests just have to be run from a certain directory? This is from the old test runner. I want to eventually remove this, but do not want to break anything. I think I will add a logging message that this is triggered, and fix any place that uses it. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:213: self._device.WaitUntilFullyBooted(timeout=120) On 2016/06/01 17:40:27, mikecase wrote: > We should probably have some shared GetDeviceBootTimeout function in > constants.py that > looks at device type and Android version and just returns a good timeout. > > At least, for the time being, would be nice to factor the 120 out to the top of > the module. Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:268: result['output'] = previous['output'] + result['output'] On 2016/06/01 17:40:28, mikecase wrote: > Do this save files ever get cleared? It looks like they will just keep on > growing forever. Is that a problem? That.. that is a bug. It should be deleted in the test_run_instance setup. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/01 17:40:28, mikecase wrote: > nit 2016 Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:27: self._STR = 'Local Device Perf Test Instance: ' On 2016/06/01 17:44:11, jbudorick wrote: > Nothing in here should be specific to a local device. Logic that is belongs in > local_device_perf_test_run.py. This is just left over debuggign information anyway. Deleted. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:32: self.steps = args.steps On 2016/06/01 17:44:11, jbudorick wrote: > 1) Why is so much of this public? It really shouldn't be. Read-only properties > would be vastly preferable. > 2) alphabetize Woops. I meant to do a pass through this and make things properties. I was being lazy while developing. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:72: On 2016/06/01 17:44:11, jbudorick wrote: > nit: only one line between member functions Done. https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... build/android/test_runner.py:712: return test_run.RunTests() On 2016/06/01 17:40:28, mikecase wrote: > Not sure I 100% sure I get what this does. Talked offline about it. Also, before landing this I plan to back out some of these changes so that it wont go live when this lands. https://codereview.chromium.org/2012323002/diff/180001/build/android/test_run... build/android/test_runner.py:857: 'perf', On 2016/06/01 17:40:28, mikecase wrote: > super nit: alphabetize Done.
PS6 comments, still not complete https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:184: cwd = os.path.abspath(os.path.join(host_paths.DIR_SOURCE_ROOT, os.pardir)) On 2016/06/01 20:32:05, rnephew (Reviews Here) wrote: > On 2016/06/01 17:40:28, mikecase wrote: > > Im not a fan of things like this. Probably would be nice to work to remove the > > need for this. So some tests just have to be run from a certain directory? > > This is from the old test runner. I want to eventually remove this, but do not > want to break anything. I think I will add a logging message that this is > triggered, and fix any place that uses it. In general, I'd prefer to make these breaking sanity changes/fixes in the transition to platform mode. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:41: """A file-like class for keeping the buildbot alive.""" What calls are taking so long that this is necessary? timeout_retry.py does something like this already, so perhaps this isn't necessary any more? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): I don't think this should be part of the default operating mode. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:124: logging.info('Unmapping device ports.') We should be doing unmapping at TearDown time, not SetUp time. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:126: self._device.RestartAdbd() I'm wondering if this is still necessary. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:136: logging.warning(msg) This should not log if you're already raising an exception. It should instead be logged where the exception is caught. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:346: if not devices_path: What is going on here with the double devices_path check? https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:375: raise NotImplementedError('No tests found!') NotImplementedError is the wrong exception type here. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:112: return 'Perf' nit: 'perf'
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:41: """A file-like class for keeping the buildbot alive.""" On 2016/06/01 20:46:06, jbudorick wrote: > What calls are taking so long that this is necessary? > > timeout_retry.py does something like this already, so perhaps this isn't > necessary any more? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... This was part of the old test runner, it might not be required. I'll take it out and see how it reacts. My guess is that the page cycler tests were causing problems. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): On 2016/06/01 20:46:06, jbudorick wrote: > I don't think this should be part of the default operating mode. It currently is, and for this CL I would like runtime behavior to match old behavior as close as possible. I will add a flag to this though, that defaults to True, to write the buildbot json. Then we can make changes to the recipes to enable it then I can default it to not write the buildbot json. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:126: self._device.RestartAdbd() On 2016/06/01 20:46:06, jbudorick wrote: > I'm wondering if this is still necessary. I'll experiment locally. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:112: return 'Perf' On 2016/06/01 20:46:06, jbudorick wrote: > nit: 'perf' I capitalized it because of the old usage here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/test...
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > On 2016/06/01 20:46:06, jbudorick wrote: > > I don't think this should be part of the default operating mode. > > It currently is, and for this CL I would like runtime behavior to match old > behavior as close as possible. I will add a flag to this though, that defaults > to True, to write the buildbot json. Then we can make changes to the recipes to > enable it then I can default it to not write the buildbot json. to reiterate: maintaining old crufty behavior is a non-goal of platform mode. It's going to be behind a flag to begin with anyway.
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): On 2016/06/01 21:03:20, jbudorick wrote: > On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > > On 2016/06/01 20:46:06, jbudorick wrote: > > > I don't think this should be part of the default operating mode. > > > > It currently is, and for this CL I would like runtime behavior to match old > > behavior as close as possible. I will add a flag to this though, that defaults > > to True, to write the buildbot json. Then we can make changes to the recipes > to > > enable it then I can default it to not write the buildbot json. > > to reiterate: maintaining old crufty behavior is a non-goal of platform mode. > It's going to be behind a flag to begin with anyway. By my last comment I just meant I wanted the flag to be on by default at first. Make a flag for the output. Land this. Make changes to recipes to add the flag. Default the flag to off. That way we do not cause any problems on the bots.
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): On 2016/06/01 21:12:13, rnephew (Reviews Here) wrote: > On 2016/06/01 21:03:20, jbudorick wrote: > > On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > > > On 2016/06/01 20:46:06, jbudorick wrote: > > > > I don't think this should be part of the default operating mode. > > > > > > It currently is, and for this CL I would like runtime behavior to match old > > > behavior as close as possible. I will add a flag to this though, that > defaults > > > to True, to write the buildbot json. Then we can make changes to the recipes > > to > > > enable it then I can default it to not write the buildbot json. > > > > to reiterate: maintaining old crufty behavior is a non-goal of platform mode. > > It's going to be behind a flag to begin with anyway. > > By my last comment I just meant I wanted the flag to be on by default at first. > Make a flag for the output. > Land this. > Make changes to recipes to add the flag. > Default the flag to off. > > That way we do not cause any problems on the bots. and I meant that this entire CL should be behind --enable-platform-mode, which will also not cause bot problems. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:112: return 'Perf' On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > On 2016/06/01 20:46:06, jbudorick wrote: > > nit: 'perf' > > I capitalized it because of the old usage here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/test... > Why do we do that? Where is it used? What if we change it? https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... build/android/test_runner.py:696: def RunPerfTestsInPlatformMode(args): Why can't this use the existing RunTestsInPlatformMode? Can we move the differences down into the various implementations of RunTests? https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... build/android/test_runner.py:827: if command == 'perf': ... and to go along with that comment, these two lines shouldn't be here.
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: def _WriteBuildBotJson(self): On 2016/06/01 21:16:09, jbudorick wrote: > On 2016/06/01 21:12:13, rnephew (Reviews Here) wrote: > > On 2016/06/01 21:03:20, jbudorick wrote: > > > On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > > > > On 2016/06/01 20:46:06, jbudorick wrote: > > > > > I don't think this should be part of the default operating mode. > > > > > > > > It currently is, and for this CL I would like runtime behavior to match > old > > > > behavior as close as possible. I will add a flag to this though, that > > defaults > > > > to True, to write the buildbot json. Then we can make changes to the > recipes > > > to > > > > enable it then I can default it to not write the buildbot json. > > > > > > to reiterate: maintaining old crufty behavior is a non-goal of platform > mode. > > > It's going to be behind a flag to begin with anyway. > > > > By my last comment I just meant I wanted the flag to be on by default at > first. > > Make a flag for the output. > > Land this. > > Make changes to recipes to add the flag. > > Default the flag to off. > > > > That way we do not cause any problems on the bots. > > and I meant that this entire CL should be behind --enable-platform-mode, which > will also not cause bot problems. Oh, yeah. I planned to back out the changes in test_runner.py that make it go live before landing. I can land the flag in this cl and have it be a no-op until platform mode is enabled. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:112: return 'Perf' On 2016/06/01 21:16:09, jbudorick wrote: > On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > > On 2016/06/01 20:46:06, jbudorick wrote: > > > nit: 'perf' > > > > I capitalized it because of the old usage here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/test... > > > > Why do we do that? Where is it used? What if we change it? I didn't dig into the LogFull() code yet, I'll look into that and see if lowercasing it when it was capitalized before will cause any issues.
Addressed most comments up til now, except for the ones I commented on saying that its in progress. https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:184: cwd = os.path.abspath(os.path.join(host_paths.DIR_SOURCE_ROOT, os.pardir)) On 2016/06/01 20:46:06, jbudorick wrote: > On 2016/06/01 20:32:05, rnephew (Reviews Here) wrote: > > On 2016/06/01 17:40:28, mikecase wrote: > > > Im not a fan of things like this. Probably would be nice to work to remove > the > > > need for this. So some tests just have to be run from a certain directory? > > > > This is from the old test runner. I want to eventually remove this, but do not > > want to break anything. I think I will add a logging message that this is > > triggered, and fix any place that uses it. > > In general, I'd prefer to make these breaking sanity changes/fixes in the > transition to platform mode. Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:41: """A file-like class for keeping the buildbot alive.""" On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > On 2016/06/01 20:46:06, jbudorick wrote: > > What calls are taking so long that this is necessary? > > > > timeout_retry.py does something like this already, so perhaps this isn't > > necessary any more? > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... > > This was part of the old test runner, it might not be required. I'll take it out > and see how it reacts. My guess is that the page cycler tests were causing > problems. Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:124: logging.info('Unmapping device ports.') On 2016/06/01 20:46:06, jbudorick wrote: > We should be doing unmapping at TearDown time, not SetUp time. Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:136: logging.warning(msg) On 2016/06/01 20:46:06, jbudorick wrote: > This should not log if you're already raising an exception. It should instead be > logged where the exception is caught. Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:346: if not devices_path: On 2016/06/01 20:46:06, jbudorick wrote: > What is going on here with the double devices_path check? Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:375: raise NotImplementedError('No tests found!') On 2016/06/01 20:46:06, jbudorick wrote: > NotImplementedError is the wrong exception type here. Done. https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:112: return 'Perf' On 2016/06/01 21:23:17, rnephew (Reviews Here) wrote: > On 2016/06/01 21:16:09, jbudorick wrote: > > On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > > > On 2016/06/01 20:46:06, jbudorick wrote: > > > > nit: 'perf' > > > > > > I capitalized it because of the old usage here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/test... > > > > > > > Why do we do that? Where is it used? What if we change it? > > I didn't dig into the LogFull() code yet, I'll look into that and see if > lowercasing it when it was capitalized before will cause any issues. Done. https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... build/android/test_runner.py:696: def RunPerfTestsInPlatformMode(args): On 2016/06/01 21:16:09, jbudorick wrote: > Why can't this use the existing RunTestsInPlatformMode? Can we move the > differences down into the various implementations of RunTests? Working on unifying them, will be in the next patchset. https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... build/android/test_runner.py:827: if command == 'perf': On 2016/06/01 21:16:09, jbudorick wrote: > ... and to go along with that comment, these two lines shouldn't be here. Only here until I finally am ready to submit, then I'll back it out. Adding [DO NOT SUBMIT] to the name so I dont accidently forget.
Description was changed from ========== [Android] Implement perf tests to platform mode. BUG=615157 ========== to ========== [Android] Implement perf tests to platform mode. BUG=615157 ==========
https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:126: self._device.RestartAdbd() On 2016/06/01 21:01:39, rnephew (Reviews Here) wrote: > On 2016/06/01 20:46:06, jbudorick wrote: > > I'm wondering if this is still necessary. > > I'll experiment locally. Local experimentation shows no detriment by getting rid of it. Moved it to run only when a test fails. https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/220001/build/android/test_run... build/android/test_runner.py:696: def RunPerfTestsInPlatformMode(args): On 2016/06/01 22:14:15, rnephew (Reviews Here) wrote: > On 2016/06/01 21:16:09, jbudorick wrote: > > Why can't this use the existing RunTestsInPlatformMode? Can we move the > > differences down into the various implementations of RunTests? > > Working on unifying them, will be in the next patchset. Done.
Patchset #9 (id:260001) has been deleted
https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:5: import io ? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:9: import pickle uh oh https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:30: _BOOTUP_TIMEOUT = 60 * 2 ? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:67: def _CleanupOutputDirectory(self): This either shouldn't be its own function or it should be defined immediately next to where it's used. Leaning toward the former. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:76: self._output_dir = tempfile.mkdtemp() This seems like the wrong place for this. Also, with the way you currently have the retry logic structured, this creates an output directory on each try but only clears the output directory after all tries. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:86: None if self._test_instance.no_timeout When would we want no timeout? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:90: logfile = sys.stdout Why is this set here and not in the GetCmdStatusAndOutputWithTimeout call? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:97: exit_code, output = cmd_helper.GetCmdStatusAndOutputWithTimeout( I'm somewhat worried about muxed output here with sys.stdout going to tests on all threads. Also, couldn't we just print the output afterward rather than using logfile? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:106: def _ProcessTestResult( Why doesn't _RunSingleTest handle both running the test and processing the result? i.e., why doesn't it call _ProcessTestResult? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:153: while exit_code != 0 and tries_left > 0: Internal retry handling, interesting. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:155: self._ResetWatcher() This already happens in _TestSetUp? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:158: cmd, exit_code, output, json_output = self._RunSingleTest(test) It looks like most of the logic below this should be in a finally block. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:167: finally: Do we really want a shard to die on exception, especially when retries are handled internally? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:199: def _ResetWatcher(self): This doesn't seem like it needs to be a separate function. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:209: def _ScreenCheck(self): Again, doesn't need to be a separate function. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:213: def _BatteryTempCheck(self): Leaning toward the two battery checks not needing their own functions, either. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:256: if self._test_instance.steps: What happens if neither of these is set? https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:259: assert steps['version'] == 1 This should be an exception, but not an assertion. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:264: for test in test_dict['steps']: for test, test_config in test_dict['steps'].iteritems(): affinity = test_config['device_affinity'] ... Also, there are a lot of possible KeyErrors in here. We should handle those a bit more gracefully. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:286: nit: one too many empty lines https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:288: def _RunOutputJsonList(self): This seems like it could be in the test instance... https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:298: def _RunPrintStep(self): ... same. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:320: local_device_test_run.NoTestsError() this needs to raise the exception it's creating https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:327: threads.append(reraiser_thread.ReraiserThread(new_shard.RunTestsOnShard)) Thinking about the parallelizer here. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:332: try: These should be handled on each thread, not on the main one. Also, you may be interested in @local_device_test_run.handle_shard_failures. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:31: def _GetChromiumRevision(): I'm not sure this is something the test runner should be responsible for. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:35: See: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... Drop the URL onto its own line and see if you still need the line-too-long disable. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:67: self._get_output_dir_archive = args.get_output_dir_archive If this argument is the path to the output directory, it should be named as such. "get_output_dir_archive" sounds like a flag. https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... build/android/test_runner.py:591: group.add_argument('single_step_command', nargs='*', action=SingleStepAction, Can you drop these onto the next line to match the way most of the other arguments in this file are formatted? https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... build/android/test_runner.py:790: _DEFAULT_PLATFORM_MODE_TESTS = [' gtest', 'instrumentation', 'perf'] Again, this CL shouldn't jump perf tests to default on.
https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:5: import io On 2016/06/07 15:29:27, jbudorick wrote: > ? io is used during the archiving of the output dir. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:9: import pickle On 2016/06/07 15:29:27, jbudorick wrote: > uh oh Talked offline. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:30: _BOOTUP_TIMEOUT = 60 * 2 On 2016/06/07 15:29:28, jbudorick wrote: > ? self._device.WaitUntilFullyBooted(timeout=_BOOTUP_TIMEOUT) I got rid of that though, and am using devil.android.tools.device_recovery now. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:67: def _CleanupOutputDirectory(self): On 2016/06/07 15:29:27, jbudorick wrote: > This either shouldn't be its own function or it should be defined immediately > next to where it's used. Leaning toward the former. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:76: self._output_dir = tempfile.mkdtemp() On 2016/06/07 15:29:27, jbudorick wrote: > This seems like the wrong place for this. > > Also, with the way you currently have the retry logic structured, this creates > an output directory on each try but only clears the output directory after all > tries. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:86: None if self._test_instance.no_timeout On 2016/06/07 15:29:26, jbudorick wrote: > When would we want no timeout? probably never, but it was a command line option in the old one so I kept it for feature parity; I can remove it. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:90: logfile = sys.stdout On 2016/06/07 15:29:28, jbudorick wrote: > Why is this set here and not in the GetCmdStatusAndOutputWithTimeout call? Artifact of previous changes, its done at the correct place now. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:97: exit_code, output = cmd_helper.GetCmdStatusAndOutputWithTimeout( On 2016/06/07 15:29:28, jbudorick wrote: > I'm somewhat worried about muxed output here with sys.stdout going to tests on > all threads. > > Also, couldn't we just print the output afterward rather than using logfile? Yeah, switched to not passing logfile. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:106: def _ProcessTestResult( On 2016/06/07 15:29:27, jbudorick wrote: > Why doesn't _RunSingleTest handle both running the test and processing the > result? i.e., why doesn't it call _ProcessTestResult? No reason it cant, just didnt do it that way; but that seems cleaner so switching to that. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:153: while exit_code != 0 and tries_left > 0: On 2016/06/07 15:29:28, jbudorick wrote: > Internal retry handling, interesting. Acknowledged. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:155: self._ResetWatcher() On 2016/06/07 15:29:26, jbudorick wrote: > This already happens in _TestSetUp? I did it afterwards so that there is actually two places for it to time out; during waiting for charinging/cooling in setup, and actually running the test. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:158: cmd, exit_code, output, json_output = self._RunSingleTest(test) On 2016/06/07 15:29:28, jbudorick wrote: > It looks like most of the logic below this should be in a finally block. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:167: finally: On 2016/06/07 15:29:27, jbudorick wrote: > Do we really want a shard to die on exception, especially when retries are > handled internally? Fixed. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:199: def _ResetWatcher(self): On 2016/06/07 15:29:28, jbudorick wrote: > This doesn't seem like it needs to be a separate function. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:209: def _ScreenCheck(self): On 2016/06/07 15:29:28, jbudorick wrote: > Again, doesn't need to be a separate function. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:213: def _BatteryTempCheck(self): On 2016/06/07 15:29:28, jbudorick wrote: > Leaning toward the two battery checks not needing their own functions, either. I felt liking doing it made the setup easier to understand/read. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:256: if self._test_instance.steps: On 2016/06/07 15:29:28, jbudorick wrote: > What happens if neither of these is set? It cannot get here without at least one of those set. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:259: assert steps['version'] == 1 On 2016/06/07 15:29:26, jbudorick wrote: > This should be an exception, but not an assertion. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:264: for test in test_dict['steps']: On 2016/06/07 15:29:28, jbudorick wrote: > for test, test_config in test_dict['steps'].iteritems(): > affinity = test_config['device_affinity'] > ... > > Also, there are a lot of possible KeyErrors in here. We should handle those a > bit more gracefully. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:286: On 2016/06/07 15:29:28, jbudorick wrote: > nit: one too many empty lines Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:288: def _RunOutputJsonList(self): On 2016/06/07 15:29:28, jbudorick wrote: > This seems like it could be in the test instance... Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:298: def _RunPrintStep(self): On 2016/06/07 15:29:28, jbudorick wrote: > ... same. I had already moved over a few things, just hadn't gotten them all. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:320: local_device_test_run.NoTestsError() On 2016/06/07 15:29:26, jbudorick wrote: > this needs to raise the exception it's creating Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:327: threads.append(reraiser_thread.ReraiserThread(new_shard.RunTestsOnShard)) On 2016/06/07 15:29:27, jbudorick wrote: > Thinking about the parallelizer here. Acknowledged. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:332: try: On 2016/06/07 15:29:26, jbudorick wrote: > These should be handled on each thread, not on the main one. > > Also, you may be interested in @local_device_test_run.handle_shard_failures. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:31: def _GetChromiumRevision(): On 2016/06/07 15:29:28, jbudorick wrote: > I'm not sure this is something the test runner should be responsible for. Any suggestion on where to move it since the buildbot json requires it? I thought about putting it in environment, but since I figured it would only ever be used by the perf tests I ended up putting it here. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:35: See: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... On 2016/06/07 15:29:28, jbudorick wrote: > Drop the URL onto its own line and see if you still need the line-too-long > disable. Yep. https://codereview.chromium.org/2012323002/diff/280001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:67: self._get_output_dir_archive = args.get_output_dir_archive On 2016/06/07 15:29:28, jbudorick wrote: > If this argument is the path to the output directory, it should be named as > such. "get_output_dir_archive" sounds like a flag. Done. https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... build/android/test_runner.py:591: group.add_argument('single_step_command', nargs='*', action=SingleStepAction, On 2016/06/07 15:29:28, jbudorick wrote: > Can you drop these onto the next line to match the way most of the other > arguments in this file are formatted? Done. Except the ones that fully fit on one line. https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... build/android/test_runner.py:790: _DEFAULT_PLATFORM_MODE_TESTS = [' gtest', 'instrumentation', 'perf'] On 2016/06/07 15:29:28, jbudorick wrote: > Again, this CL shouldn't jump perf tests to default on. Thats why I added the do not submit to the name, I do not intend to submit these; I just haven't deleted it from the patchsets yet so I dont have to keep adding and removing it for local runs (I know I could jus tuse the flags for platform mode, but I.. dont)
On 2016/06/09 22:06:43, rnephew (Reviews Here) wrote: ping
Description was changed from ========== [Android] Implement perf tests to platform mode. BUG=615157 ========== to ========== [Android] Implement perf tests to platform mode. BUG=615157, 590229 ==========
https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/280001/build/android/test_run... build/android/test_runner.py:790: _DEFAULT_PLATFORM_MODE_TESTS = [' gtest', 'instrumentation', 'perf'] On 2016/06/09 22:06:39, rnephew (Reviews Here) wrote: > On 2016/06/07 15:29:28, jbudorick wrote: > > Again, this CL shouldn't jump perf tests to default on. > > Thats why I added the do not submit to the name, I do not intend to submit > these; I just haven't deleted it from the patchsets yet so I dont have to keep > adding and removing it for local runs (I know I could jus tuse the flags for > platform mode, but I.. dont) Removed it, renaming to reflect that this is gone.
https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:36: return local_device_perf_test_run.LocalDevicePerfTestRun(env, nit: move env down & tweak indentation https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:81: forwarder.Forwarder.UnmapAllDevicePorts(self._device) Why is this unmapping but not mapping? What's misbehaving and not unmapping ports? Can we only do this if there are ports forwarded? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:95: logging.info('Running %s on shard %s', test, self._index) str(self._index) or %d? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:99: logging.info('Timeout for %s test: %s', test, timeout) str(timeout) or %d? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:106: logging.debug('Running test with command \'%s\'', cmd) nit: Use double quotes for the string when it contains single quotes s.t. you don't have to escape https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:131: # TODO(rnephew): Improve device recovery logic. Why is this in _ProcessTestResult? I think this should be in RunTestsOnShard (or in its own function called by RunTestsOnShard) https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:161: def RunTestsOnShard(self): The functions in this class are ordered strangely. Try to arrange them s.t. they appear roughly in the order they're called, e.g. RunTestsOnShard _TestSetUp _RunSingleTest _CreateCmd _ProcessTestResult _ArchiveOutputDir _SaveResult _TestTearDown https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:166: while (result_type != base_test_result.ResultType.PASS This retry mechanism is the inverse of how gtests & instrumentation tests handle retries. Is that how the old perf test runner handles them? What's the reasoning if so? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:276: devices = [device_utils.DeviceUtils(s) port over https://codereview.chromium.org/2070043002 https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:289: if self._test_instance.output_json_list: I'm wondering if these should be subcommands of 'perf' -- something like test_runner.py perf output ... test_runner.py perf print ... test_runner.py perf run ... You could conceivably have separate test_run classes for each in that case, which may be a bit cleaner if a bit more verbose. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:302: for x in xrange(min(len(self._devices), len(test_buckets))): I think you could do this as: def run_perf_tests(x): s = TestShard(self._env, self._test_instance, self._devices[x], x, test_buckets[x], ...) s.RunTestsOnShard() device_indices = range(min(len(self.devices), len(test_buckets))) shards = parallelizer.Parallelizer(device_indices).pMap(run_perf_tests) return shards.pGet(self._timeout) WDYT? I think the log messages would look like TimeoutThread-1-for-run_perf_tests(1) and I think it'd clean up a lot of the thread logic you're doing manually in here. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:303: new_shard = TestShard(self._env, self._test_instance, self._devices[x], x, I don't think we should make a shard for a device if it's blacklisted coming in. (i.e., port https://codereview.chromium.org/2084493003) https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:333: nit: +1 line https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:334: class VersionError(Exception): This name should be more specific. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_test_run.py:204: class NoTestsError(Exception): If we're raising this when we have no perf tests, we should probably also raise it when we have no gtests or instrumentation tests. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:33: # pylint: disable=line-too-long Is this still necessary w/ the URL on its own line? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:138: # XXX ??? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:161: logging.exception('Exception when reading chartjson.') We see this a lot in the current perf test runner when a device is blacklisted. Can we do something smarter here if that's the case? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:176: json.dump(data, f, sort_keys=True, indent=2, separators=(',', ': ')) Does the buildbot json need to be human-readable? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:178: def RunOutputJsonList(self): Why are RunOutputJsonList and OutputJsonList separate? Why does the latter deal in exit codes? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:188: def RunPrintStep(self): Similarly, why are RunPrintStep & PrintTestOutput separate? Are PrintTestOutput and OutputJsonList being called from somewhere else that I'm missing? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:211: def output_dir_archive_path(self): nit: alpha https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... build/android/test_runner.py:609: group.add_argument( Why are you adding these three? Are they used by some common code? (local_device_test_run?) https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... build/android/test_runner.py:804: _DEFAULT_PLATFORM_MODE_TESTS = [' gtest', 'instrumentation'] s/' gtest'/'gtest'
https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:36: return local_device_perf_test_run.LocalDevicePerfTestRun(env, On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > nit: move env down & tweak indentation Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:81: forwarder.Forwarder.UnmapAllDevicePorts(self._device) On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > Why is this unmapping but not mapping? What's misbehaving and not unmapping > ports? Can we only do this if there are ports forwarded? run_benchmark takes care of the mapping. This just makes sure nothing is mapped after it attempts to do that in case it exited badly in the last run. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:95: logging.info('Running %s on shard %s', test, self._index) On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > str(self._index) or %d? Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:99: logging.info('Timeout for %s test: %s', test, timeout) On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > str(timeout) or %d? Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:106: logging.debug('Running test with command \'%s\'', cmd) On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > nit: Use double quotes for the string when it contains single quotes s.t. you > don't have to escape Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:131: # TODO(rnephew): Improve device recovery logic. On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > Why is this in _ProcessTestResult? I think this should be in RunTestsOnShard (or > in its own function called by RunTestsOnShard) Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:161: def RunTestsOnShard(self): On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > The functions in this class are ordered strangely. Try to arrange them s.t. they > appear roughly in the order they're called, e.g. > > RunTestsOnShard > _TestSetUp > _RunSingleTest > _CreateCmd > _ProcessTestResult > _ArchiveOutputDir > _SaveResult > _TestTearDown Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:166: while (result_type != base_test_result.ResultType.PASS On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > This retry mechanism is the inverse of how gtests & instrumentation tests handle > retries. Is that how the old perf test runner handles them? What's the reasoning > if so? So that we do not have to store which tests are passed/failed. We just run through the list and when we reach the end we are done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:276: devices = [device_utils.DeviceUtils(s) On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > port over https://codereview.chromium.org/2070043002 Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:289: if self._test_instance.output_json_list: On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > I'm wondering if these should be subcommands of 'perf' -- something like > > test_runner.py perf output ... > test_runner.py perf print ... > test_runner.py perf run ... > > You could conceivably have separate test_run classes for each in that case, > which may be a bit cleaner if a bit more verbose. It could be switched to that if you want; but probably a lot of boilerplate for little benefit. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:302: for x in xrange(min(len(self._devices), len(test_buckets))): On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > I think you could do this as: > > def run_perf_tests(x): > s = TestShard(self._env, self._test_instance, self._devices[x], x, > test_buckets[x], ...) > s.RunTestsOnShard() > > device_indices = range(min(len(self.devices), len(test_buckets))) > shards = parallelizer.Parallelizer(device_indices).pMap(run_perf_tests) > return shards.pGet(self._timeout) > > WDYT? I think the log messages would look like > > TimeoutThread-1-for-run_perf_tests(1) > > and I think it'd clean up a lot of the thread logic you're doing manually in > here. Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:303: new_shard = TestShard(self._env, self._test_instance, self._devices[x], x, On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > I don't think we should make a shard for a device if it's blacklisted coming in. > (i.e., port https://codereview.chromium.org/2084493003) Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:333: On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > nit: +1 line Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:334: class VersionError(Exception): On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > This name should be more specific. Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_test_run.py:204: class NoTestsError(Exception): On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > If we're raising this when we have no perf tests, we should probably also raise > it when we have no gtests or instrumentation tests. Would you like that in this CL, or in a CL after this lands? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:33: # pylint: disable=line-too-long On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > Is this still necessary w/ the URL on its own line? Yes, it is 94 characters long. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:138: # XXX On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > ??? Note for myself to easily find that spot, accidentally left in. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:161: logging.exception('Exception when reading chartjson.') On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > We see this a lot in the current perf test runner when a device is blacklisted. > Can we do something smarter here if that's the case? I could attempt to do a recovery; but that wouldn't help anything. This is run after all tests have run. I added recovery logic if a test fails in the test run part, taht should help. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:176: json.dump(data, f, sort_keys=True, indent=2, separators=(',', ': ')) On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > Does the buildbot json need to be human-readable? Probably not. Got rid of indent. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:178: def RunOutputJsonList(self): On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > Why are RunOutputJsonList and OutputJsonList separate? Why does the latter deal > in exit codes? RunOutputJsonList is a wrapper around OutputJsonList that returns in a format that RunTestsInPlatformMode can digest. I wanted to keep the wrapping seperate from the actual work; tahts why they are separated. No real reason they have to be. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:188: def RunPrintStep(self): On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > Similarly, why are RunPrintStep & PrintTestOutput separate? > > Are PrintTestOutput and OutputJsonList being called from somewhere else that I'm > missing? Same answer as above, this just wraps it and I wanted that separate. I can merge them. Honestly I did this CL so long ago I dont know exactly the logic behind why I separated them. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:211: def output_dir_archive_path(self): On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > nit: alpha Done. https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... build/android/test_runner.py:609: group.add_argument( On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > Why are you adding these three? Are they used by some common code? > (local_device_test_run?) They are used in the test runner in RunTestsInPlatformMode https://codereview.chromium.org/2012323002/diff/320001/build/android/test_run... build/android/test_runner.py:804: _DEFAULT_PLATFORM_MODE_TESTS = [' gtest', 'instrumentation'] On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > s/' gtest'/'gtest' Done.
https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:289: if self._test_instance.output_json_list: On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > > I'm wondering if these should be subcommands of 'perf' -- something like > > > > test_runner.py perf output ... > > test_runner.py perf print ... > > test_runner.py perf run ... > > > > You could conceivably have separate test_run classes for each in that case, > > which may be a bit cleaner if a bit more verbose. > > It could be switched to that if you want; but probably a lot of boilerplate for > little benefit. Moved them to their own classes in the same file. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:25: Moved perf test runs to its own class because pylint was angry. ************* Module pylib.base.test_run_factory R: 26, 0: Too many return statements (8/6) (too-many-return-statements)
https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:81: forwarder.Forwarder.UnmapAllDevicePorts(self._device) On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > > Why is this unmapping but not mapping? What's misbehaving and not unmapping > > ports? Can we only do this if there are ports forwarded? > > run_benchmark takes care of the mapping. This just makes sure nothing is mapped > after it attempts to do that in case it exited badly in the last run. Does the instance of the Forwarder in this process even know about the ports mapped by the instance of Forwarder in run_benchmark's process? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:166: while (result_type != base_test_result.ResultType.PASS On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > > This retry mechanism is the inverse of how gtests & instrumentation tests > handle > > retries. Is that how the old perf test runner handles them? What's the > reasoning > > if so? > > So that we do not have to store which tests are passed/failed. We just run > through the list and when we reach the end we are done. sgtm https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_test_run.py:204: class NoTestsError(Exception): On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > > If we're raising this when we have no perf tests, we should probably also > raise > > it when we have no gtests or instrumentation tests. > > Would you like that in this CL, or in a CL after this lands? hm, probably a separate CL after this lands. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:33: # pylint: disable=line-too-long On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > > Is this still necessary w/ the URL on its own line? > > Yes, it is 94 characters long. Hm, ok. I'll have to investigate this separately. https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:161: logging.exception('Exception when reading chartjson.') On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > > We see this a lot in the current perf test runner when a device is > blacklisted. > > Can we do something smarter here if that's the case? > > I could attempt to do a recovery; but that wouldn't help anything. > This is run after all tests have run. I added recovery logic if a test fails in > the test run part, taht should help. It should, that's true. I'm more wondering if we can make what happened more clear if recovery doesn't help. If the device is in the blacklist when we hit this condition, maybe we exit w/ an infra error? https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:178: def RunOutputJsonList(self): On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > On 2016/06/28 10:27:33, jbudorick (EMEA til June 30) wrote: > > Why are RunOutputJsonList and OutputJsonList separate? Why does the latter > deal > > in exit codes? > > RunOutputJsonList is a wrapper around OutputJsonList that returns in a format > that RunTestsInPlatformMode can digest. I wanted to keep the wrapping seperate > from the actual work; tahts why they are separated. No real reason they have to > be. I think it's ok for them to be separate, but the inner functions really shouldn't be dealing w/ exit codes. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:25: On 2016/06/30 19:10:36, rnephew (Reviews Here) wrote: > Moved perf test runs to its own class because pylint was angry. > > ************* Module pylib.base.test_run_factory > R: 26, 0: Too many return statements (8/6) (too-many-return-statements) Hm, I thought we had that one disabled. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:26: def CreatePerfTestRun(args, env, test_instance): nit: _CreatePerfTestRun https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:63: except Exception: # pylint: disable=broad-except Do we need to catch Exception? Can we catch something narrower? What is _RunSingleTest throwing -- just devil exceptions? https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:67: if result_type != base_test_result.ResultType.PASS: Is this the right condition to attempt device recovery, or should we only be doing it on catching a device exception? https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:79: if self._output_dir: self._output_dir seems to be handled pretty strangely in this class. It's deleted here and in _TestSetUp, created in _TestSetUp, and not touched in _TestTearDown. Ideally, it would be created in _TestSetUp, deleted in _TestTearDown, and that's it. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:102: if not self._device.IsOnline(): Wouldn't we run into issues earlier in this function if this were the case? Should this be the first device function in _TestSetUp? https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:105: if self._output_dir: Following from the above, I don't think this should be here. If self._output_dir isn't None here, then we have a problem w/ the teardown logic. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: if self._watcher: self._watcher gets reset twice in here? Also, the watcher no longer seems to be used at all. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:117: None if self._test_instance.no_timeout I *think* this is redundant, as the test run's already checks test_instance.no_timeout https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:132: except cmd_helper.TimeoutError as e: It'd be good if this could be propagated down to results processing somehow s.t. the result_type was TIMEOUT instead of the more generic FAIL. I also think you also need to define end_time in here. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:147: nit: -1 line https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:224: self._test_instance = test_instance nit: alpha https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:234: self._watcher = watchdog_timer.WatchdogTimer(self._timeout) again, I don't think this is used any more. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:236: if (not (self._test_instance.print_step Do you still need this if check? Neither of these should be true, right? https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:246: # From where this is called one of these two must be set. If this is the case, we should fail hard in the event that it's not true. As currently written, _SplitTestsByAffinity would hit an unclear error when trying to do test_dict['steps'].iteritems() w/ test_dict = None. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:275: logging.exception('Bad test config') This should include more detail -- at least the bad test config itself, if not exactly what lookup failed https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:319: return list(itertools.chain.from_iterable(shards.pGet(self._timeout))) 🔥 https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:350: def _CreateShards(self, _tests): Do these need to be here? https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_test_run.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_test_run.py:203: +1 line https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:108: except Exception: # pylint: disable=broad-except similar to the previous file: What is this catching? Can we catch something a bit narrower? https://codereview.chromium.org/2012323002/diff/360001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/test_run... build/android/test_runner.py:805: def RunTestsCommand(args): # pylint: disable=too-many-return-statements nit: +2 blank lines https://codereview.chromium.org/2012323002/diff/360001/tools/perf/page_sets/t... File tools/perf/page_sets/typical_10_mobile.py (right): https://codereview.chromium.org/2012323002/diff/360001/tools/perf/page_sets/t... tools/perf/page_sets/typical_10_mobile.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. I'm assuming you didn't mean to include these changes?
Patchset #14 (id:380001) has been deleted
https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:26: def CreatePerfTestRun(args, env, test_instance): On 2016/07/01 14:20:10, jbudorick wrote: > nit: _CreatePerfTestRun Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:63: except Exception: # pylint: disable=broad-except On 2016/07/01 14:20:10, jbudorick wrote: > Do we need to catch Exception? Can we catch something narrower? What is > _RunSingleTest throwing -- just devil exceptions? I want it to catch just about anything so that it can run device_recovery and attempt to run again. I could probably narrow it down to just devil exceptions though; since they are the most likely things. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:67: if result_type != base_test_result.ResultType.PASS: On 2016/07/01 14:20:10, jbudorick wrote: > Is this the right condition to attempt device recovery, or should we only be > doing it on catching a device exception? I think that would not catch the issue where the forwarder is messed up. There are probably other cases as well. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:79: if self._output_dir: On 2016/07/01 14:20:10, jbudorick wrote: > self._output_dir seems to be handled pretty strangely in this class. It's > deleted here and in _TestSetUp, created in _TestSetUp, and not touched in > _TestTearDown. Ideally, it would be created in _TestSetUp, deleted in > _TestTearDown, and that's it. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:102: if not self._device.IsOnline(): On 2016/07/01 14:20:10, jbudorick wrote: > Wouldn't we run into issues earlier in this function if this were the case? > Should this be the first device function in _TestSetUp? Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:105: if self._output_dir: On 2016/07/01 14:20:10, jbudorick wrote: > Following from the above, I don't think this should be here. If self._output_dir > isn't None here, then we have a problem w/ the teardown logic. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:110: if self._watcher: On 2016/07/01 14:20:10, jbudorick wrote: > self._watcher gets reset twice in here? > > Also, the watcher no longer seems to be used at all. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:117: None if self._test_instance.no_timeout On 2016/07/01 14:20:10, jbudorick wrote: > I *think* this is redundant, as the test run's already checks > test_instance.no_timeout Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:132: except cmd_helper.TimeoutError as e: On 2016/07/01 14:20:10, jbudorick wrote: > It'd be good if this could be propagated down to results processing somehow s.t. > the result_type was TIMEOUT instead of the more generic FAIL. > > I also think you also need to define end_time in here. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:147: On 2016/07/01 14:20:10, jbudorick wrote: > nit: -1 line Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:224: self._test_instance = test_instance On 2016/07/01 14:20:10, jbudorick wrote: > nit: alpha Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:234: self._watcher = watchdog_timer.WatchdogTimer(self._timeout) On 2016/07/01 14:20:10, jbudorick wrote: > again, I don't think this is used any more. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:236: if (not (self._test_instance.print_step On 2016/07/01 14:20:10, jbudorick wrote: > Do you still need this if check? Neither of these should be true, right? Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:246: # From where this is called one of these two must be set. On 2016/07/01 14:20:10, jbudorick wrote: > If this is the case, we should fail hard in the event that it's not true. As > currently written, _SplitTestsByAffinity would hit an unclear error when trying > to do test_dict['steps'].iteritems() w/ test_dict = None. Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:275: logging.exception('Bad test config') On 2016/07/01 14:20:10, jbudorick wrote: > This should include more detail -- at least the bad test config itself, if not > exactly what lookup failed Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:319: return list(itertools.chain.from_iterable(shards.pGet(self._timeout))) On 2016/07/01 14:20:10, jbudorick wrote: > 🔥 Acknowledged. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:350: def _CreateShards(self, _tests): On 2016/07/01 14:20:10, jbudorick wrote: > Do these need to be here? Yep. Linter errors. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_test_run.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_test_run.py:203: On 2016/07/01 14:20:10, jbudorick wrote: > +1 line Done. https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:108: except Exception: # pylint: disable=broad-except On 2016/07/01 14:20:10, jbudorick wrote: > similar to the previous file: What is this catching? Can we catch something a > bit narrower? Just using what the old one had; I think KeyError is the most likely one; so I will narrow it down to that and if more pop up we can add them. https://codereview.chromium.org/2012323002/diff/360001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/test_run... build/android/test_runner.py:805: def RunTestsCommand(args): # pylint: disable=too-many-return-statements On 2016/07/01 14:20:10, jbudorick wrote: > nit: +2 blank lines Done. https://codereview.chromium.org/2012323002/diff/360001/tools/perf/page_sets/t... File tools/perf/page_sets/typical_10_mobile.py (right): https://codereview.chromium.org/2012323002/diff/360001/tools/perf/page_sets/t... tools/perf/page_sets/typical_10_mobile.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/01 14:20:10, jbudorick wrote: > I'm assuming you didn't mean to include these changes? Yeah, no.
only a few comments https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:76: self._TestTearDown() I think* since self.TestSetUp is called in the inner "while: try:" loop and this is called in the outer try:, that if a test is retried SetUp will be called multiple times but TearDown will only be called once. I think this will cause multiple output_dirs to be created (it is created in SetUp, but only one will be deleted) https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:151: actual_exit_code = exit_code Should the fact that you are setting exit_code to be -1 if it is None be reflected in actual_exit_code. Like should this line be before that? https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:263: if len(self._test_buckets) < affinity + 1: You could make self._test_buckets a dict, like... self._test_buckets = {1: {}, 2: {}, 4: {}} Just an option, keeping it as-is (as a list indexed by affinities) might just be easier. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:292: # Affinitize the tests. We should have some documentation on what "Affinitize" is and why we do it. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:336: class LocalDevicePerfTestRunOutputJsonList(LocalDevicePerfTestRun): what a name :D https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:37: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... I think I mentioned this before (I didnt see if you respsonded), but I think this link will go bad if the runtests.py file is updated? I think so anyway.
https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:76: self._TestTearDown() On 2016/07/01 23:29:44, mikecase wrote: > I think* since self.TestSetUp is called in the inner "while: try:" loop and this > is called in the outer try:, that if a test is retried SetUp will be called > multiple times but TearDown will only be called once. I think this will cause > multiple output_dirs to be created (it is created in SetUp, but only one will be > deleted) Yeah, I put this in the wrong finally block. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:151: actual_exit_code = exit_code On 2016/07/01 23:29:44, mikecase wrote: > Should the fact that you are setting exit_code to be -1 if it is None be > reflected in actual_exit_code. Like should this line be before that? In porting the old code I saw this and figured None == error while running == -1 https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:263: if len(self._test_buckets) < affinity + 1: On 2016/07/01 23:29:44, mikecase wrote: > You could make self._test_buckets a dict, like... self._test_buckets = {1: {}, > 2: {}, 4: {}} > > Just an option, keeping it as-is (as a list indexed by affinities) might just be > easier. Yeah, I prefer it to just be lists of lists of tests. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:292: # Affinitize the tests. On 2016/07/01 23:29:44, mikecase wrote: > We should have some documentation on what "Affinitize" is and why we do it. Done. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:336: class LocalDevicePerfTestRunOutputJsonList(LocalDevicePerfTestRun): On 2016/07/01 23:29:44, mikecase wrote: > what a name :D Acknowledged. https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/400001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:37: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... On 2016/07/01 23:29:44, mikecase wrote: > I think I mentioned this before (I didnt see if you respsonded), but I think > this link will go bad if the runtests.py file is updated? I think so anyway. Yeah, I dont think there is any way around that though.
https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/320001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:81: forwarder.Forwarder.UnmapAllDevicePorts(self._device) On 2016/07/01 14:20:09, jbudorick wrote: > On 2016/06/29 22:27:19, rnephew (Reviews Here) wrote: > > On 2016/06/28 10:27:32, jbudorick (EMEA til June 30) wrote: > > > Why is this unmapping but not mapping? What's misbehaving and not unmapping > > > ports? Can we only do this if there are ports forwarded? > > > > run_benchmark takes care of the mapping. This just makes sure nothing is > mapped > > after it attempts to do that in case it exited badly in the last run. > > Does the instance of the Forwarder in this process even know about the ports > mapped by the instance of Forwarder in run_benchmark's process? ^ https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:67: if result_type != base_test_result.ResultType.PASS: On 2016/07/01 22:09:32, rnephew (Reviews Here) wrote: > On 2016/07/01 14:20:10, jbudorick wrote: > > Is this the right condition to attempt device recovery, or should we only be > > doing it on catching a device exception? > > I think that would not catch the issue where the forwarder is messed up. There > are probably other cases as well. Acknowledged. Fine with this for now, but we should keep an eye on it. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:31: return local_device_perf_test_run.LocalDevicePerfTestRunOutputJsonList( nit: Case kinda has a point, these are comically long. Can we shorten them to local_device_perf_test_run.PrintStep and local_device_perf_test_run.OutputJsonList ? https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:60: except device_errors.CommandFailedError: We should either: - have this catch device_errors.BaseError - add another exception handler for device_errors.CommandTimeoutError that sets result_type = ResultType.TIMEOUT I'm leaning toward the second. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:72: result = base_test_result.TestRunResults() nit: blank line before this one https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:74: self._results.append(result) A list of single-result TestRunResults objects seems a bit weird. Also, why is self._results an instance variable? It looks like it can be local to this function. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:100: def _RunSingleTest(self, test): The sequencing in this function is a bit odd. I think it should be buildbot json stuff used by the GetCmdStatusAndOutputWithTimeout call log stuff used by the GetCmdStatusAndOutputWithTimeout call try: start time run end time json output except: https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:103: timeout = self._tests[test].get('timeout', self._timeout) timeout is created here but not used (beyond logging) until the GetCmdStatusAndOutputWithTimeout call below https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:106: cmd = self._CreateCmd(test) cmd is created here but not used at all until the try block https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:111: logging.debug("Running test with command '%s'", cmd) Why is this in the try block? https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:123: timed_out = True Can we just deal with the ResultType explicitly? https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:236: raise PerfTestRunGetStepsError( Just raise this at the end of the function w/o the if guard. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:271: return self._test_buckets Why is this creating an instance variable and returning it? https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:16: from devil.utils import cmd_helper nit: this should precede pylib https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:88: def OutputJsonList(self): Both this and PrintTestOutput are still dealing with exit codes :( Why can't they just return a ResultType? https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:108: except KeyError: # pylint: disable=broad-except nit: is broad-except still necessary here? https://codereview.chromium.org/2012323002/diff/420001/tools/perf/page_sets/p... File tools/perf/page_sets/power_cases.py (left): https://codereview.chromium.org/2012323002/diff/420001/tools/perf/page_sets/p... tools/perf/page_sets/power_cases.py:28: action_runner.Wait(10) Are these supposed to be in this CL?
https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/ba... File build/android/pylib/base/test_run_factory.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/ba... build/android/pylib/base/test_run_factory.py:31: return local_device_perf_test_run.LocalDevicePerfTestRunOutputJsonList( On 2016/07/06 19:12:10, jbudorick wrote: > nit: Case kinda has a point, these are comically long. Can we shorten them to > local_device_perf_test_run.PrintStep and > local_device_perf_test_run.OutputJsonList ? Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:60: except device_errors.CommandFailedError: On 2016/07/06 19:12:10, jbudorick wrote: > We should either: > - have this catch device_errors.BaseError > - add another exception handler for device_errors.CommandTimeoutError that sets > result_type = ResultType.TIMEOUT > > I'm leaning toward the second. Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:72: result = base_test_result.TestRunResults() On 2016/07/06 19:12:10, jbudorick wrote: > nit: blank line before this one Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:74: self._results.append(result) On 2016/07/06 19:12:10, jbudorick wrote: > A list of single-result TestRunResults objects seems a bit weird. > > Also, why is self._results an instance variable? It looks like it can be local > to this function. Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:100: def _RunSingleTest(self, test): On 2016/07/06 19:12:10, jbudorick wrote: > The sequencing in this function is a bit odd. I think it should be > > buildbot json > stuff used by the GetCmdStatusAndOutputWithTimeout call > log stuff used by the GetCmdStatusAndOutputWithTimeout call > try: > start time > run > end time > json output > except: Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:111: logging.debug("Running test with command '%s'", cmd) On 2016/07/06 19:12:10, jbudorick wrote: > Why is this in the try block? Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:123: timed_out = True On 2016/07/06 19:12:10, jbudorick wrote: > Can we just deal with the ResultType explicitly? Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:236: raise PerfTestRunGetStepsError( On 2016/07/06 19:12:10, jbudorick wrote: > Just raise this at the end of the function w/o the if guard. Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:271: return self._test_buckets On 2016/07/06 19:12:10, jbudorick wrote: > Why is this creating an instance variable and returning it? Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:16: from devil.utils import cmd_helper On 2016/07/06 19:12:10, jbudorick wrote: > nit: this should precede pylib Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:88: def OutputJsonList(self): On 2016/07/06 19:12:10, jbudorick wrote: > Both this and PrintTestOutput are still dealing with exit codes :( > > Why can't they just return a ResultType? Done. https://codereview.chromium.org/2012323002/diff/420001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:108: except KeyError: # pylint: disable=broad-except On 2016/07/06 19:12:10, jbudorick wrote: > nit: is broad-except still necessary here? Done. https://codereview.chromium.org/2012323002/diff/420001/tools/perf/page_sets/p... File tools/perf/page_sets/power_cases.py (left): https://codereview.chromium.org/2012323002/diff/420001/tools/perf/page_sets/p... tools/perf/page_sets/power_cases.py:28: action_runner.Wait(10) On 2016/07/06 19:12:10, jbudorick wrote: > Are these supposed to be in this CL? Bah, no.
ping.
lgtm with nits, but probably want John to give it one more look https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:218: nit: consider factoring out 60 * 60 into DEFAULT_PERF_TEST_RUN_TIMEOUT or something https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:231: # From where this is called one of these two must be set. I think it would be clearer if it was written.... if {} elif {} else { raise PerfTestRunGetStepsError } https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:287: def RunTests(self): #override? https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:297: def run_perf_tests(x): nit: would prefer shard_index or shard_id or affinity_id over variable name x. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:333: def SetUp(self): not:override https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:336: def RunTests(self): nit: #override? https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:353: def SetUp(self): nit: override https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:356: def RunTests(self): nit: #override? https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:37: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... You can optionally pick a specific revision to link to instead of just master, like.... https://chromium.googlesource.com/chromium/tools/build/+/387e3cf3/scripts/sla... but yeah, it's fine as is.
https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:245: if steps['version'] != 1: Nit: constify this version since you use it here and in the exception below. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:253: # This splits tests by their device affinity; so that the same tests always nit: no ; https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:77: self._single_step = args.single_step nit: Why isn't this just: self._single_step = ' '.join(args.single_step_command) if args.single_step else None eliminating the if args.single_step line above? https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:122: return exit_codes.INFRA This needs to raise an infra error, not return w/ the infra exit code.
https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:218: On 2016/07/12 18:33:14, mikecase wrote: > nit: consider factoring out 60 * 60 into DEFAULT_PERF_TEST_RUN_TIMEOUT or > something Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:287: def RunTests(self): On 2016/07/12 18:33:14, mikecase wrote: > #override? Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:297: def run_perf_tests(x): On 2016/07/12 18:33:14, mikecase wrote: > nit: would prefer shard_index or shard_id or affinity_id over variable name x. Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:333: def SetUp(self): On 2016/07/12 18:33:14, mikecase wrote: > not:override Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:336: def RunTests(self): On 2016/07/12 18:33:14, mikecase wrote: > nit: #override? Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:353: def SetUp(self): On 2016/07/12 18:33:14, mikecase wrote: > nit: override Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:356: def RunTests(self): On 2016/07/12 18:33:14, mikecase wrote: > nit: #override? Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:37: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... On 2016/07/12 18:33:14, mikecase wrote: > You can optionally pick a specific revision to link to instead of just master, > like.... > > https://chromium.googlesource.com/chromium/tools/build/+/387e3cf3/scripts/sla... > > but yeah, it's fine as is. Done.
https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:245: if steps['version'] != 1: On 2016/07/12 18:45:10, jbudorick wrote: > Nit: constify this version since you use it here and in the exception below. Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:253: # This splits tests by their device affinity; so that the same tests always On 2016/07/12 18:45:10, jbudorick wrote: > nit: no ; Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... File build/android/pylib/perf/perf_test_instance.py (right): https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:77: self._single_step = args.single_step On 2016/07/12 18:45:10, jbudorick wrote: > nit: Why isn't this just: > > self._single_step = ' '.join(args.single_step_command) if args.single_step > else None > > eliminating the if args.single_step line above? Done. https://codereview.chromium.org/2012323002/diff/440001/build/android/pylib/pe... build/android/pylib/perf/perf_test_instance.py:122: return exit_codes.INFRA On 2016/07/12 18:45:10, jbudorick wrote: > This needs to raise an infra error, not return w/ the infra exit code. Done.
lgtm
The CQ bit was checked by rnephew@chromium.org
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/2012323002/#ps480001 (title: "johns 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 ========== [Android] Implement perf tests to platform mode. BUG=615157, 590229 ========== to ========== [Android] Implement perf tests to platform mode. BUG=615157, 590229 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Implement perf tests to platform mode. BUG=615157, 590229 ========== to ========== [Android] Implement perf tests to platform mode. BUG=615157, 590229 Committed: https://crrev.com/8db92cc2828e9757b91662a1ed7057f17f8024ed Cr-Commit-Position: refs/heads/master@{#404978} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/8db92cc2828e9757b91662a1ed7057f17f8024ed Cr-Commit-Position: refs/heads/master@{#404978} |
