Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
323073007:
Android: Cleaned up python setup steps for gtests. (Closed)
|
Created:
6 years, 6 months ago by Victor Starodub Modified:
6 years, 5 months ago CC:
bulach, chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid: Cleaned up python setup steps for gtests.
Installation attempted one time per device. No complex
setup(pushing data/bringing up servers) for listing tests.
BUG=383410
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281520
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed comments from Marcus. #
Total comments: 1
Messages
Total messages: 25 (0 generated)
lgtm, but please wait for john. thanks!! small suggestions for the description: Android: summary <80cols \n Long description\n with multiple lines <80cols. (i.e., put an "Android" prefix so sheriffs will know it's an android-specific patch, then a short summary, \n, long description). https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:235: """ nit: raise NotImplementedError() https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:239: "dummy", base_test_result.ResultType.PASS) nit: s/"/'/
I can't say I'm a fan of installing the test package in one place for gtests and in another for all of the other tests. (Perhaps we should look at installing test packages earlier for all tests?) Calls to adb are definitely responsible for the delay outside of apk installation. How many of those does this patch actually save, and approximately how much time does that save? As far as I can tell, it's just one call to AndroidCommands.ManagedInstall, which, with an unchanged apk on the second call, is probably ~5 calls. https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:357: _InstallPackage(test_options, test_package, devices) I believe this is the "complex setup" you're referring to. We're still pushing the same data (and we have to). The only difference is that it's no longer in _GetTests. Since _GetTests _depends_ on the apk or executable being on the device, I'm not sure that's a great idea.
On 2014/06/11 18:22:58, jbudorick wrote: > I can't say I'm a fan of installing the test package in one place for gtests and > in another for all of the other tests. (Perhaps we should look at installing > test packages earlier for all tests?) > > Calls to adb are definitely responsible for the delay outside of apk > installation. How many of those does this patch actually save, and approximately > how much time does that save? > > As far as I can tell, it's just one call to AndroidCommands.ManagedInstall, > which, with an unchanged apk on the second call, is probably ~5 calls. > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > File build/android/pylib/gtest/setup.py (right): > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > build/android/pylib/gtest/setup.py:357: _InstallPackage(test_options, > test_package, devices) > I believe this is the "complex setup" you're referring to. We're still pushing > the same data (and we have to). The only difference is that it's no longer in > _GetTests. Since _GetTests _depends_ on the apk or executable being on the > device, I'm not sure that's a great idea. What we save: 1. If we have two or more devices, we will essentially wait for installation twice. Once for listing tests and once for running them (there won't be an apk on 2nd device). 2. For content_browsertests we re-run these md5-summing steps every test, accumulating quite some time over the whole run. 3. Regarding complex setup: currently, if test requires an http server, we would also bring it up for listing tests and we will also try to push the test data. Note that any "adb am/pm" command usually takes ~0.8s, so adb commands can be more costly than expected. For invoking a single test from content_browsertests, it reduces overhead to get a running test activity from 30s to 17s (not counting actual package installation process, which is ~15s). If using a couple of attached devices we also don't wait for installation twice. It doesn't result in major savings on bots though, since bottlenecks are a bit different (file transfer is very slow there due to some reason, significantly slower than bandwidth/number of devices).
Replied to John's comment. https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:357: _InstallPackage(test_options, test_package, devices) We're just installing the package, we don't try push data here or bring up http server as we did, so that's the "complex setup" we don't do. Since it's the code of the 'Setup' method, I think relying on the fact that the package will be installed inside it is ok. Also it looks fine to me that we do depend on the order of setup steps being correct. A lot of things during test run can depend on the package being installed, but I don't think it necessary means that we need to put self.test_package.Install() calls everywhere.
On 2014/06/11 18:51:50, Victor Starodub wrote: > On 2014/06/11 18:22:58, jbudorick wrote: > > I can't say I'm a fan of installing the test package in one place for gtests > and > > in another for all of the other tests. (Perhaps we should look at installing > > test packages earlier for all tests?) > > > > Calls to adb are definitely responsible for the delay outside of apk > > installation. How many of those does this patch actually save, and > approximately > > how much time does that save? > > > > As far as I can tell, it's just one call to AndroidCommands.ManagedInstall, > > which, with an unchanged apk on the second call, is probably ~5 calls. > > > > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > > File build/android/pylib/gtest/setup.py (right): > > > > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > > build/android/pylib/gtest/setup.py:357: _InstallPackage(test_options, > > test_package, devices) > > I believe this is the "complex setup" you're referring to. We're still pushing > > the same data (and we have to). The only difference is that it's no longer in > > _GetTests. Since _GetTests _depends_ on the apk or executable being on the > > device, I'm not sure that's a great idea. > > What we save: > 1. If we have two or more devices, we will essentially wait for installation > twice. Once for listing tests and > once for running them (there won't be an apk on 2nd device). Oh right, the sharding. I don't really like hijacking the test_dispatcher to do things other than run tests :/ > 2. For content_browsertests we re-run these md5-summing steps every test, > accumulating quite some time over the > whole run. Where is that being called every test? It looks like it should only happen once per device in BaseTestRunner.SetUp, which is the only place that calls InstallTestPackage. (It appears to me that we md5sum the command line file on the device every test, but I'm not sure how to get around that off the top of my head.) > 3. Regarding complex setup: currently, if test requires an http server, we would > also bring it up for listing > tests and we will also try to push the test data. > Yeah, I just saw where/how this is happening. It looks like it installs the package, too. Good catch. > Note that any "adb am/pm" command usually takes ~0.8s, so adb commands can be > more costly than expected. Yeah, that's about what I expect. There's been talk about doing something to mitigate that. > > For invoking a single test from content_browsertests, it reduces overhead to get > a running test activity from > 30s to 17s (not counting actual package installation process, which is ~15s). If In contrast, I did not expect this at all. Wow. > using a couple of attached > devices we also don't wait for installation twice. It doesn't result in major > savings on bots though, since > bottlenecks are a bit different (file transfer is very slow there due to some > reason, significantly slower > than bandwidth/number of devices).
On 2014/06/11 19:13:29, Victor Starodub wrote: > Replied to John's comment. > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > File build/android/pylib/gtest/setup.py (right): > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > build/android/pylib/gtest/setup.py:357: _InstallPackage(test_options, > test_package, devices) > We're just installing the package, we don't try push data here or bring up http > server as we did, so that's the "complex setup" we don't do. > > Since it's the code of the 'Setup' method, I think relying on the fact that the > package will be installed inside it is ok. Also it looks fine to me that we do > depend on the order of setup steps being correct. > My point is that splitting them makes it easier to screw up the order in a future patch, but I guess that's fair. > A lot of things during test run can depend on the package being installed, but I > don't think it necessary means that we need to put self.test_package.Install() > calls everywhere. Agreed. I guess my primary question is if there's a good way to do this across all test types, not just gtest.
https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:270: class TestListerRunner(_TestSetupRunner): One other thought, since we were talking about this: what's would be the difference in execution time between this change and just adding noop overrides of SetUp and TearDown to the existing TestListerRunner (i.e., what you did in _TestSetupRunner above)?
Sorry for taking so long to get back to the patch. Fixed comments from Marcus. John -- I'm hesitant to touch all the tests at once. I suggest working on gtests first and when we're sure what would be the pattern for test setup/package installation/etc, we could work on other test types and cleaning up the code. What do you think? https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:235: """ On 2014/06/11 16:59:11, bulach wrote: > nit: raise NotImplementedError() Done. https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:239: "dummy", base_test_result.ResultType.PASS) On 2014/06/11 16:59:11, bulach wrote: > nit: s/"/'/ Done. https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... build/android/pylib/gtest/setup.py:270: class TestListerRunner(_TestSetupRunner): Second install attempt would be ~2.5s if the package is already installed (1 device), at least ~15s for a 30Mb package if the package is not installed (2nd device).
lgtm https://codereview.chromium.org/323073007/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/323073007/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:256: test_dispatcher.RunTests(['setup'], TestInstallerRunnerFactory, devices, I still think that using test_dispatcher as a way to set up devices is abusive. That said, the speed gains you've described are not trivial, we're already using test_dispatcher in this abusive way in here, and I don't have a better solution at the moment.
On 2014/07/02 14:48:18, Victor Starodub wrote: > Sorry for taking so long to get back to the patch. > > Fixed comments from Marcus. > > John -- I'm hesitant to touch all the tests at once. I suggest working on gtests > first and when we're sure what would be the pattern for test setup/package > installation/etc, we could work on other test types and cleaning up the code. > What do you think? > sgtm > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > File build/android/pylib/gtest/setup.py (right): > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > build/android/pylib/gtest/setup.py:235: """ > On 2014/06/11 16:59:11, bulach wrote: > > nit: raise NotImplementedError() > > Done. > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > build/android/pylib/gtest/setup.py:239: "dummy", > base_test_result.ResultType.PASS) > On 2014/06/11 16:59:11, bulach wrote: > > nit: s/"/'/ > > Done. > > https://codereview.chromium.org/323073007/diff/1/build/android/pylib/gtest/se... > build/android/pylib/gtest/setup.py:270: class > TestListerRunner(_TestSetupRunner): > Second install attempt would be ~2.5s if the package is already installed (1 > device), at least ~15s for a 30Mb package if the package is not installed (2nd > device).
The CQ bit was checked by starodub@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/starodub@google.com/323073007/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by starodub@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/starodub@google.com/323073007/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by starodub@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/starodub@google.com/323073007/20001
Message was sent while issue was closed.
Change committed as 281520
Message was sent while issue was closed.
On 2014/07/07 11:35:00, I haz the power (commit-bot) wrote: > Change committed as 281520 Reverting in https://codereview.chromium.org/371343002, see crbug/392117 |
