New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 671413 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

run_chrome_public_test_apk should attempt to rebuild dependent targets when run locally

Project Member Reported by mariakho...@chromium.org, Dec 6 2016

Issue description

Repro:
1. Have some code that fails tests running on a device
2. Fix the code to pass the test
3. Run run_chrome_public_test_apk -f "ExternalNavigationDelegateImplTest.*", for example, to check that tests have been fixed

Expected:
A new chrome_public_apk is pushed to the device by the test script

Actual:
Tests run on the old code and continue to fail

 
Components: Test>Android
Labels: -Pri-3 Pri-2
GN args:

use_goma = true
target_cpu = "arm"
is_component_build = true
is_debug = true
goma_dir = "/usr/local/google/goma/goma"
target_os = "android"

$ cr build chrome to build (this builds chrome_apk, not chrome_public_apk, maybe that's the issue?)

Then this to test:
$ ./out_android/Debug/bin/run_chrome_public_test_apk -v -f "ExternalNavigationDelegateImplTest.*"

cr build chrome could definitely be the issue, though I admittedly know very little about cr.
 - does cr use out_android (or do you have it configured to do so)?
 - What are the timestamps on the contents of out_android/Debug/apks/?
Yes, cr uses out_android by default and sets the out dir variables accordingly
I am guessing maybe the issue is that out_android/Debug/apks might not even have ChromePublic.apk at all since I haven't built it. What happens in that case?
That definitely *should* die with an error, but I'm not sure that that's the case.
Die with an error would be an improvement. But ideally, build and push apk would be even better.
Building is currently outside of the scope of the test runner, but that's both an interesting and (I think) feasible idea.
I checked, and it does fail with an error if the APK doesn't exist, but it's not very clear. I've uploaded https://codereview.chromium.org/2552033003/ to clarify the error message.

You're sure that ChromePublic.apk/ChromePublicTest.apk don't exist in that directory? We build the runner script (here, run_chrome_public_test_apk) as part of the test apk target, so if the runner exists, both the test apk and the apk under test should also exist.
No, I am not actually sure whether they existed or not. They definitely exist now. I can try to reproduce again and check.
$ ls -l out_android/Debug/apks

-rw-r----- 1 mariakhomenko eng     6084 Nov 30 14:31 ApkWithWebApkService.apk
-rw-r----- 1 mariakhomenko eng 84317762 Dec  6 10:47 Chrome.apk
-rw-r----- 1 mariakhomenko eng 84158867 Dec  5 16:09 ChromePublic.apk
-rw-r----- 1 mariakhomenko eng  1821205 Dec  5 14:46 ChromePublicTest.apk
-rw-r----- 1 mariakhomenko eng  2481339 Dec  2 16:46 ChromePublicTestSupport.apk
-rw-r----- 1 mariakhomenko eng   257409 Sep 28 11:23 ChromeTest.apk
-rw-r----- 1 mariakhomenko eng 12050386 Dec  2 16:46 ChromiumNetTestSupport.apk
-rw-r----- 1 mariakhomenko eng  8227614 Nov 28 11:01 CustomTabsClientExample.apk
-rw-r----- 1 mariakhomenko eng    93803 Dec  2 16:46 DexOptimizer.apk
-rw-r----- 1 mariakhomenko eng   117730 Sep 28 11:08 OnDeviceInstrumentationDriver.apk


Then edit source code (ExternalNavigationDelegateImpl.java) such that tests would fail.
Then run 
$ ./out_android/Debug/bin/run_chrome_public_test_apk -f "ExternalNavigationDelegateImplTest.*"

All tests pass, even though they should fail.

$ ls -l out_android/Debug/apks
total 189016
-rw-r----- 1 mariakhomenko eng     6084 Nov 30 14:31 ApkWithWebApkService.apk
-rw-r----- 1 mariakhomenko eng 84317762 Dec  6 10:47 Chrome.apk
-rw-r----- 1 mariakhomenko eng 84158867 Dec  5 16:09 ChromePublic.apk
-rw-r----- 1 mariakhomenko eng  1821205 Dec  5 14:46 ChromePublicTest.apk
-rw-r----- 1 mariakhomenko eng  2481339 Dec  2 16:46 ChromePublicTestSupport.apk
-rw-r----- 1 mariakhomenko eng   257409 Sep 28 11:23 ChromeTest.apk
-rw-r----- 1 mariakhomenko eng 12050386 Dec  2 16:46 ChromiumNetTestSupport.apk
-rw-r----- 1 mariakhomenko eng  8227614 Nov 28 11:01 CustomTabsClientExample.apk
-rw-r----- 1 mariakhomenko eng    93803 Dec  2 16:46 DexOptimizer.apk
-rw-r----- 1 mariakhomenko eng   117730 Sep 28 11:08 OnDeviceInstrumentationDriver.apk


Question: is the issue that I am not rebuilding run_chrome_public_test_apk in the middle? I wouldn't have thought to need to do that...

$ ls -l out_android/Debug/bin/
total 16
drwxr-x--- 2 mariakhomenko eng 4096 Nov 23 14:26 helper
-rwxr-x--- 1 mariakhomenko eng 1264 Dec  5 16:08 run_chrome_public_test_apk
-rwxr-x--- 1 mariakhomenko eng 1041 Sep 28 11:10 run_chrome_test_apk
-rwxr-x--- 1 mariakhomenko eng  881 Nov 15 12:08 run_components_unittests



No, the issue is that you're not rebuilding chrome_public_test_apk in the middle. We don't attempt to reinstall the APK because the APK hasn't changed.
It also didn't occur to me to rebuild a test apk because I didn't change any test code.
er, right, sorry. chrome_public_apk, not chrome_public_test_apk.
Yep, that's fair. I verified that if I did rebuild chrome_public_apk, then it's pushed.

What happened is that I built chrome_apk instead of chrome_public_apk because that's what I usually build and hence the confusion.
Cc: jbudorick@chromium.org
Labels: -Type-Bug Type-Feature
Owner: mikec...@chromium.org
Summary: run_chrome_public_test_apk should attempt to rebuild dependent targets when run locally (was: run_chrome_public_test_apk should push the latest code to the device)
Yeah, and that's understandable. I think the best way to avoid that confusion may be to do what you suggested in #6, so I'm updating this bug to reflect that. We'll have to be careful about how it behaves on bots, though.

tentatively assigning to mikecase, who did the wrapper scripts originally.
Project Member

Comment 16 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d0973803e7802215f26802941eb4bb2f0c97037

commit 0d0973803e7802215f26802941eb4bb2f0c97037
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Tue Dec 06 20:28:23 2016

Roll src/third_party/catapult/ 9902da68b..cb066006a (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/9902da68b6de..cb066006a85e

$ git log 9902da68b..cb066006a --date=short --no-merges --format='%ad %ae %s'
2016-12-06 jbudorick [devil] Clarify DeviceUtils.Install{,SplitApk} failure on missing APK.

BUG=671413

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2554993002
Cr-Commit-Position: refs/heads/master@{#436694}

[modify] https://crrev.com/0d0973803e7802215f26802941eb4bb2f0c97037/DEPS

Sign in to add a comment