Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200193002:
[Android] Add ability to run deviceless tests on hosts without devices for perf tests. (Closed)
|
Created:
4 years, 4 months ago by rnephew (Reviews Here) Modified:
4 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add ability to run deviceless tests on hosts without devices for perf tests.
Rationale: The resource_size test does not require a device to run. We want to be able
to run this test on the builders, which do not have devices. This CL allows for it to be
run without a device by setting the affinity to -1. We use the android test runner to run
these because of the ability to send the results to the perf dashboard.
BUG=609365
Committed: https://crrev.com/85867a0b2091b0bbdb2a734b41cd1e7a8a1ad610
Cr-Commit-Position: refs/heads/master@{#410429}
Patch Set 1 #Patch Set 2 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 10
Patch Set 3 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 2
Patch Set 4 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 3
Patch Set 5 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #Patch Set 6 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 8
Patch Set 7 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 4
Patch Set 8 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #
Total comments: 8
Patch Set 9 : [Android] Add ability to run deviceless tests on hosts without devices for perf tests. #Messages
Total messages: 34 (13 generated)
Description was changed from ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. BUG=609365 ========== to ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ==========
rnephew@chromium.org changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org, mikecase@chromium.org
This is essentially creating a second kind of TestShard. What if you did so explicitly?
On 2016/08/02 17:13:40, jbudorick wrote: > This is essentially creating a second kind of TestShard. What if you did so > explicitly? Done.
https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:65: logging.info('Create host shard for the following tests:') This shouldn't be saying it's a host shard. https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:69: self._device = None Why is this still here rather than in DeviceTestShard? https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:355: if affinity == -1: Could this allow for multiple host test shards by just handling negative numbers as host shards? https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:393: if shard_id == -1: nit: I'd prefer if shard_id == -1: s = HostTestShard(...) else: if device_status.IsBlacklisted(...): ... s = DeviceTestShard(...) return s.RunTestsOnShard() https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:395: self._no_device_tests, retries=3, timeout=self._timeout) nit: indentation is off
https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:65: logging.info('Create host shard for the following tests:') On 2016/08/02 18:51:50, jbudorick wrote: > This shouldn't be saying it's a host shard. Done. https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:69: self._device = None On 2016/08/02 18:51:50, jbudorick wrote: > Why is this still here rather than in DeviceTestShard? The CreateCmd function needs it. I decided to put it in here because I didn't want both other test runners to have nearly identical methods with only one line of code difference. https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:355: if affinity == -1: On 2016/08/02 18:51:50, jbudorick wrote: > Could this allow for multiple host test shards by just handling negative numbers > as host shards? Probably, but I'm not sure there is much advantage in allowing for more than one thread doing on-host tests. There aren't many of them. https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:393: if shard_id == -1: On 2016/08/02 18:51:50, jbudorick wrote: > nit: I'd prefer > > if shard_id == -1: > s = HostTestShard(...) > else: > if device_status.IsBlacklisted(...): > ... > s = DeviceTestShard(...) > return s.RunTestsOnShard() Done. https://codereview.chromium.org/2200193002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:395: self._no_device_tests, retries=3, timeout=self._timeout) On 2016/08/02 18:51:50, jbudorick wrote: > nit: indentation is off Done.
https://codereview.chromium.org/2200193002/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:111: if self._device: If you don't want to duplicate code, then just have a _ExtraCmdArgs method that returns an empty string by default and returns '--device %s' for DeviceTestShard. (You may also want to look at handling cmd as a list until the end of this function, then doing a ' '.join(cmd))
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2200193002/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_perf_test_run.py:111: if self._device: On 2016/08/02 19:06:54, jbudorick wrote: > If you don't want to duplicate code, then just have a _ExtraCmdArgs method that > returns an empty string by default and returns '--device %s' for > DeviceTestShard. > > (You may also want to look at handling cmd as a list until the end of this > function, then doing a ' '.join(cmd)) Done.
perezju@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:71: self._index = -1 can we use "None" for no device needed? https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:112: cmd.extend([self._tests[test]['cmd']]) nit: cmd.append if you're just adding one thing https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:378: if affinity == -1: ditto, can we use None instead of -1?
On 2016/08/04 15:10:50, perezju wrote: > https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... > File build/android/pylib/local/device/local_device_perf_test_run.py (right): > > https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_perf_test_run.py:71: self._index = > -1 > can we use "None" for no device needed? > > https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_perf_test_run.py:112: > cmd.extend([self._tests[test]['cmd']]) > nit: cmd.append if you're just adding one thing > > https://codereview.chromium.org/2200193002/diff/100001/build/android/pylib/lo... > build/android/pylib/local/device/local_device_perf_test_run.py:378: if affinity > == -1: > ditto, can we use None instead of -1? Ok, now both putting 'null' and not having the line at all will yield it being run on a host test shard.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
rietveld is being weird. What about the use of -1 in RunTests?
On 2016/08/04 17:50:54, jbudorick wrote: > rietveld is being weird. > > What about the use of -1 in RunTests? It was still working because of it appending -1 to the device index list. I changed it to append None and check for None.
https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:86: test, cmd, str(self._index), timeout) This is liable to look weird: "Running foo with command 'bar' on shard None with timeout 123" https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:124: def _ExtendPersistedResult(self, # pylint: disable=no-self-use Can you put the lint disable inside the function's scope? This looks really weird as-is. https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:254: if (self._test_instance.collect_chartjson_data There's commonality between the device and host versions of _TestSetUp; could you extract it into TestShard and then have both implementations call the superclass version? https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:315: if self._output_dir: Same comment as _TestSetUp
https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:86: test, cmd, str(self._index), timeout) On 2016/08/04 19:00:39, jbudorick wrote: > This is liable to look weird: "Running foo with command 'bar' on shard None with > timeout 123" Done. https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:124: def _ExtendPersistedResult(self, # pylint: disable=no-self-use On 2016/08/04 19:00:39, jbudorick wrote: > Can you put the lint disable inside the function's scope? This looks really > weird as-is. Done. https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:254: if (self._test_instance.collect_chartjson_data On 2016/08/04 19:00:39, jbudorick wrote: > There's commonality between the device and host versions of _TestSetUp; could > you extract it into TestShard and then have both implementations call the > superclass version? Done. https://codereview.chromium.org/2200193002/diff/180001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:315: if self._output_dir: On 2016/08/04 19:00:39, jbudorick wrote: > Same comment as _TestSetUp Done.
Description was changed from ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ========== to ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ==========
Description was changed from ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ========== to ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ==========
Just a couple super-nits. Thanks 1,000,000 for doing this! lgtm https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:118: cmd.extend(['echo']) nit: cmd.append('echo') https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:379: affinity = test_config.get('device_affinity', None) nit: None is the default for .get(). More common to not explicitly pass it I think.
https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:118: cmd.extend(['echo']) On 2016/08/05 03:37:09, agrieve wrote: > nit: cmd.append('echo') Done. https://codereview.chromium.org/2200193002/diff/200001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:379: affinity = test_config.get('device_affinity', None) On 2016/08/05 03:37:09, agrieve wrote: > nit: None is the default for .get(). More common to not explicitly pass it I > think. Done.
thanks! lgtm, just a few tiny nits. https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:119: cmd.extend([self._tests[test]['cmd']]) nit: extend -> append :) https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:128: pass nit: I think you can raise NotImplementedError here, then you don't need to disable the pylint warning. https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:136: pass nit: same here https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:248: nit: remove extra line
https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_perf_test_run.py (right): https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:119: cmd.extend([self._tests[test]['cmd']]) On 2016/08/05 11:02:57, perezju wrote: > nit: extend -> append :) Done. https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:128: pass On 2016/08/05 11:02:57, perezju wrote: > nit: I think you can raise NotImplementedError here, then you don't need to > disable the pylint warning. Done. https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:136: pass On 2016/08/05 11:02:57, perezju wrote: > nit: same here Done. https://codereview.chromium.org/2200193002/diff/220001/build/android/pylib/lo... build/android/pylib/local/device/local_device_perf_test_run.py:248: On 2016/08/05 11:02:57, perezju wrote: > nit: remove extra line 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 agrieve@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/2200193002/#ps240001 (title: "[Android] Add ability to run deviceless tests on hosts without devices for perf tests.")
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] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ========== to ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 ========== to ========== [Android] Add ability to run deviceless tests on hosts without devices for perf tests. Rationale: The resource_size test does not require a device to run. We want to be able to run this test on the builders, which do not have devices. This CL allows for it to be run without a device by setting the affinity to -1. We use the android test runner to run these because of the ability to send the results to the perf dashboard. BUG=609365 Committed: https://crrev.com/85867a0b2091b0bbdb2a734b41cd1e7a8a1ad610 Cr-Commit-Position: refs/heads/master@{#410429} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/85867a0b2091b0bbdb2a734b41cd1e7a8a1ad610 Cr-Commit-Position: refs/heads/master@{#410429} |
