Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551353004:
Add helper script for running CTS APKs stored in the GS buckets. (Closed)
|
Created:
4 years ago by mikecase (-- gone --) Modified:
4 years ago CC:
android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd helper script for running CTS APKs stored in the GS buckets.
Would like to have this script replace the logic in recipes that
downloads the CTS zips, unzips it, and runs it via tradefed.
Committed: https://crrev.com/5c5e3f0e689128dceb35368731d6c7db8d85b4f0
Cr-Commit-Position: refs/heads/master@{#438642}
Patch Set 1 #Patch Set 2 : Add helper script for running CTS APKs stored in the GS buckets. #
Total comments: 6
Patch Set 3 : Add helper script for running CTS APKs stored in the GS buckets. #Patch Set 4 : Add helper script for running CTS APKs stored in the GS buckets. #
Total comments: 11
Patch Set 5 : Add helper script for running CTS APKs stored in the GS buckets. #
Total comments: 13
Patch Set 6 : Add helper script for running CTS APKs stored in the GS buckets. #
Total comments: 4
Messages
Total messages: 29 (6 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org, sgurun@chromium.org, yolandyan@chromium.org
What do you think of something like this... It is a script that will (1) Download the CTS APK from GS. It only downloads the APK and not the entire CTS zip. (2) Runs the test with the build/android/test_runner.py. It does not use tradefed. This script would be used by the bots and would also let devs run the same CTS APKs as the bots locally. Usage is like... ./run_cts.py --arch arm_64 --platform M ...to run the arm_64 M CTS tests. It also forwards all test runner args. So you can do things like... ./run_cts.py --arch arm_64 --platform N --verbose -f cts.example.Class#ExampleTest
https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/c... File android_webview/tools/cts_config/webview_cts_gcs_path.json (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/c... android_webview/tools/cts_config/webview_cts_gcs_path.json:5: "apk": "arm_64/L/CtsWebkitTestCases.apk", This should definitely stored in a sub directory with the build number as its name https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... android_webview/tools/run_cts.py:63: temp_dir = None hmm, I wonder if we can store CTS apk in a third_party repo instead of downloading them each time for run, because I would really like to use this manually
https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... android_webview/tools/run_cts.py:63: temp_dir = None On 2016/12/08 at 19:16:03, the real yoland wrote: > hmm, I wonder if we can store CTS apk in a third_party repo instead of downloading them each time for run, because I would really like to use this manually How about an optionally --apk-dir arg. If specified it will look for and cache the APKs there? If not specified, it will delete the APK after running.
ty for the review yoland :D https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/c... File android_webview/tools/cts_config/webview_cts_gcs_path.json (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/c... android_webview/tools/cts_config/webview_cts_gcs_path.json:5: "apk": "arm_64/L/CtsWebkitTestCases.apk", On 2016/12/08 at 19:16:03, the real yoland wrote: > This should definitely stored in a sub directory with the build number as its name yup, agree. Fixed. https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... android_webview/tools/run_cts.py:63: temp_dir = None On 2016/12/08 at 19:17:58, mikecase wrote: > On 2016/12/08 at 19:16:03, the real yoland wrote: > > hmm, I wonder if we can store CTS apk in a third_party repo instead of downloading them each time for run, because I would really like to use this manually > > How about an optionally --apk-dir arg. If specified it will look for and cache the APKs there? If not specified, it will delete the APK after running. CTS are now cached locally if you specify apk-dir arg. Otherwise, they are downloaded to a temp_dir and deleted.
If I can land this, will change bots with this CL to use this script to run tests... https://chromium-review.googlesource.com/#/c/418355/
On 2016/12/09 at 20:06:15, mikecase wrote: > If I can land this, will change bots with this CL to use this script to run tests... > https://chromium-review.googlesource.com/#/c/418355/ Sweet, I have a bug hanging for CTS not able to find aapt path on WebView N bot, this would just skip over the problem
Sorry about the delay here. I'll try to look over this today.
I just realized how small the APKs are and downloading them is so fast that it would have made no sense to store them anyway https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/20001/android_webview/tools/r... android_webview/tools/run_cts.py:63: temp_dir = None On 2016/12/08 at 19:17:58, mikecase wrote: > On 2016/12/08 at 19:16:03, the real yoland wrote: > > hmm, I wonder if we can store CTS apk in a third_party repo instead of downloading them each time for run, because I would really like to use this manually > > How about an optionally --apk-dir arg. If specified it will look for and cache the APKs there? If not specified, it will delete the APK after running. ya, make sense, not everyone need CTS, third_party repo might be a bit much https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:35: os.path.dirname(__file__), 'cts_config', 'expected_failure_on_bot.json') maybe use DIR_SOURCE_ROOT instead of relative path from this file's location? not sure though https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:80: [_GSUTIL_PATH, 'cp', google_storage_cts_path, local_cts_path]): would using tools/depot_tools/download_from_google_storage.py be easier? https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:101: required=True, maybe default this if there is only one option now and in the near future https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:109: '--skip-expected-failures', default this to true? https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:118: args, test_runner_args = parser.parse_known_args() TIL :O I assume these args aren't needed in test_runner.py (at least for CTS) then?
This is super sweet! Should help out a lot in situations like this (b/ 33012998#comment25) , maybe next step to also add some device status check in the script (e.g. if the --platform is set as N but the phone itself is L) but this can wait until next iteration
https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:35: os.path.dirname(__file__), 'cts_config', 'expected_failure_on_bot.json') On 2016/12/09 at 20:56:36, the real yoland wrote: > maybe use DIR_SOURCE_ROOT instead of relative path from this file's location? > > not sure though Yeah, in order to do that I would have to import something from pylib. I could do this, since I already have to add build/android to PYTHONPATH for something else. I dont think it would make the code cleaner though. Leaving as is for now https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:80: [_GSUTIL_PATH, 'cp', google_storage_cts_path, local_cts_path]): On 2016/12/09 at 20:56:36, the real yoland wrote: > would using tools/depot_tools/download_from_google_storage.py be easier? IIRC, download_from_google_storage.py only downloads files using SHA1 as filenames. I don't think it will work for this case. https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:101: required=True, On 2016/12/09 at 20:56:36, the real yoland wrote: > maybe default this if there is only one option now and in the near future agreed, Done https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:109: '--skip-expected-failures', On 2016/12/09 at 20:56:36, the real yoland wrote: > default this to true? hmm, leaving as False so people aren't confused as to why some tests aren't running https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:118: args, test_runner_args = parser.parse_known_args() On 2016/12/09 at 20:56:36, the real yoland wrote: > TIL :O > I assume these args aren't needed in test_runner.py (at least for CTS) then? Yeah, this is to allow forwarding some args to the test runner that people might want to use. Like -f to filter tests, -v for verbose, or --results-json-file to get results in recipes. I guess just have to make sure no args added to this script that share a name with a test_runner.py arg :/
https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:35: os.path.dirname(__file__), 'cts_config', 'expected_failure_on_bot.json') On 2016/12/09 at 20:56:36, the real yoland wrote: > maybe use DIR_SOURCE_ROOT instead of relative path from this file's location? > > not sure though Yeah, in order to do that I would have to import something from pylib. I could do this, since I already have to add build/android to PYTHONPATH for something else. I dont think it would make the code cleaner though. Leaving as is for now https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:80: [_GSUTIL_PATH, 'cp', google_storage_cts_path, local_cts_path]): On 2016/12/09 at 20:56:36, the real yoland wrote: > would using tools/depot_tools/download_from_google_storage.py be easier? IIRC, download_from_google_storage.py only downloads files using SHA1 as filenames. I don't think it will work for this case. https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:101: required=True, On 2016/12/09 at 20:56:36, the real yoland wrote: > maybe default this if there is only one option now and in the near future agreed, Done https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:109: '--skip-expected-failures', On 2016/12/09 at 20:56:36, the real yoland wrote: > default this to true? hmm, leaving as False so people aren't confused as to why some tests aren't running https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:118: args, test_runner_args = parser.parse_known_args() On 2016/12/09 at 20:56:36, the real yoland wrote: > TIL :O > I assume these args aren't needed in test_runner.py (at least for CTS) then? Yeah, this is to allow forwarding some args to the test runner that people might want to use. Like -f to filter tests, -v for verbose, or --results-json-file to get results in recipes. I guess just have to make sure no args added to this script that share a name with a test_runner.py arg :/
lgtm https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/60001/android_webview/tools/r... android_webview/tools/run_cts.py:80: [_GSUTIL_PATH, 'cp', google_storage_cts_path, local_cts_path]): On 2016/12/12 at 17:31:43, mikecase wrote: > On 2016/12/09 at 20:56:36, the real yoland wrote: > > would using tools/depot_tools/download_from_google_storage.py be easier? > > IIRC, download_from_google_storage.py only downloads files using SHA1 as filenames. I don't think it will work for this case. right right, I remember now
https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/p... File android_webview/tools/pylintrc (right): https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/p... android_webview/tools/pylintrc:18: locally-enabled, remove this https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:16: # pylint: disable=import-error Disable this on a line-by-line basis and remove the enable= https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:41: """Gets relative path CTS APK is stored.""" nit: this sounds like you're missing a word or two. https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:84: # TODO(mikecase): This doesn't work at all with the -f and --gtest-filter are the same? https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:90: [_TEST_RUNNER_PATH] + ['instrumentation'] + test_runner_args) nit: [_TEST_RUNNER_PATH, 'instrumentation'] + test_runner_args https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:100: choices=['arm_64'], Just arm64?
https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:100: choices=['arm_64'], On 2016/12/12 at 18:40:56, jbudorick wrote: > Just arm64? I believe this is so because the recipe module function argument itself is "arm_64".
@sgurun, could you take a quick look at this. Will need OWNER for android_webview/tools/ to land this CL. Thanks https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/p... File android_webview/tools/pylintrc (right): https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/p... android_webview/tools/pylintrc:18: locally-enabled, On 2016/12/12 at 18:40:56, jbudorick wrote: > remove this Done https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:16: # pylint: disable=import-error On 2016/12/12 at 18:40:56, jbudorick wrote: > Disable this on a line-by-line basis and remove the enable= Done https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:41: """Gets relative path CTS APK is stored.""" On 2016/12/12 at 18:40:56, jbudorick wrote: > nit: this sounds like you're missing a word or two. Done. https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:84: # TODO(mikecase): This doesn't work at all with the On 2016/12/12 at 18:40:56, jbudorick wrote: > -f and --gtest-filter are the same? yeah, custom -f option wont work with --skip-expected-failures. The custom -f arg will just overwrite the -f arg written by the --skip-expected-failures. https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:90: [_TEST_RUNNER_PATH] + ['instrumentation'] + test_runner_args) On 2016/12/12 at 18:40:56, jbudorick wrote: > nit: [_TEST_RUNNER_PATH, 'instrumentation'] + test_runner_args Done https://codereview.chromium.org/2551353004/diff/80001/android_webview/tools/r... android_webview/tools/run_cts.py:100: choices=['arm_64'], On 2016/12/12 at 18:40:56, jbudorick wrote: > Just arm64? Probably should be, but since I store the CTS APKs in gs://chromium-cts/arm_64, it is easiest this way.
https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... android_webview/tools/run_cts.py:99: choices=['arm_64'], does this only work for 64 bits? how about L platform, do we have a L device there?
https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... android_webview/tools/run_cts.py:99: choices=['arm_64'], On 2016/12/14 at 01:20:58, sgurun wrote: > does this only work for 64 bits? how about L platform, do we have a L device there? Yes, one can run CTS L with this script. This arch is used for CTS test package, not meant for device architecture I think the "arm_64" is based on what we got from Android Build. From android build, even for L platform, the CTS artifacts are separated into cts_arm_64, cts_misp, cts_x86_64. And I believe tests apks themselves have no different for 32bits and 64bits
https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... android_webview/tools/run_cts.py:83: # TODO(mikecase): This doesn't work at all with the Oh, I think I understand what you mean now -- if someone calling this script specifies their own -f/--gtest-filter/etc flag and --skip-expected-failures, they conflict and only the one that appears last on the command-line is used. Should this script check that a user only specifies one or the other but not both?
On 2016/12/14 at 16:22:19, jbudorick wrote: > https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... > File android_webview/tools/run_cts.py (right): > > https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... > android_webview/tools/run_cts.py:83: # TODO(mikecase): This doesn't work at all with the > Oh, I think I understand what you mean now -- if someone calling this script specifies their own -f/--gtest-filter/etc flag and --skip-expected-failures, they conflict and only the one that appears last on the command-line is used. > > Should this script check that a user only specifies one or the other but not both? Yeah, or we could change the -f option to accept multiple uses and have it merge the filters somehow.
lgtm https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... File android_webview/tools/run_cts.py (right): https://codereview.chromium.org/2551353004/diff/100001/android_webview/tools/... android_webview/tools/run_cts.py:99: choices=['arm_64'], On 2016/12/14 01:29:29, the real yoland wrote: > On 2016/12/14 at 01:20:58, sgurun wrote: > > does this only work for 64 bits? how about L platform, do we have a L device > there? > > Yes, one can run CTS L with this script. This arch is used for CTS test package, > not meant for device architecture > > I think the "arm_64" is based on what we got from Android Build. From android > build, even for L platform, the CTS artifacts are separated into cts_arm_64, > cts_misp, cts_x86_64. And I believe tests apks themselves have no different for > 32bits and 64bits yeah the tests should be in Java all.
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yolandyan@chromium.org Link to the patchset: https://codereview.chromium.org/2551353004/#ps100001 (title: "Add helper script for running CTS APKs stored in the GS buckets.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481751725810170,
"parent_rev": "8b2e3e9e41bc97a890d6e4eb6c1d1aee7856a705", "commit_rev":
"dc6cedbdb109a4f3be157b78da7a4d71a2eff072"}
Message was sent while issue was closed.
Description was changed from ========== Add helper script for running CTS APKs stored in the GS buckets. Would like to have this script replace the logic in recipes that downloads the CTS zips, unzips it, and runs it via tradefed. ========== to ========== Add helper script for running CTS APKs stored in the GS buckets. Would like to have this script replace the logic in recipes that downloads the CTS zips, unzips it, and runs it via tradefed. Review-Url: https://codereview.chromium.org/2551353004 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add helper script for running CTS APKs stored in the GS buckets. Would like to have this script replace the logic in recipes that downloads the CTS zips, unzips it, and runs it via tradefed. Review-Url: https://codereview.chromium.org/2551353004 ========== to ========== Add helper script for running CTS APKs stored in the GS buckets. Would like to have this script replace the logic in recipes that downloads the CTS zips, unzips it, and runs it via tradefed. Committed: https://crrev.com/5c5e3f0e689128dceb35368731d6c7db8d85b4f0 Cr-Commit-Position: refs/heads/master@{#438642} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c5e3f0e689128dceb35368731d6c7db8d85b4f0 Cr-Commit-Position: refs/heads/master@{#438642} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
