Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576213002:
[Offline Pages] Improvements on test harness scripts. (Closed)
|
Created:
4 years ago by romax Modified:
4 years ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Improvements on test harness scripts.
Made --use-test-scheduler as default; Ensure adb server is started before
copying files and showing config when running the harness. Also fixed a
bug where external dir wasn't used.
BUG=NONE
Committed: https://crrev.com/0546f6445943373803e79f07b7c99e50909af964
Cr-Commit-Position: refs/heads/master@{#438926}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change the adb path to relative. #Messages
Total messages: 14 (6 generated)
romax@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
Changes made to test harness script, PTAL!
https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:96: adb_path = os.path.join(os.getcwd(), This hardcodes the assumption that we are being run from the chromium/src directory. Do we already have a limitation that we have to run from there? If not, it might be good not to introduce a limitation. If so, then this is fine as is. Also, do we know for sure this is the right version of ADB to use? We could end up with the device updating, and the version of adb under third_party not getting updated right away, and we might want to be able to override it. If we don't know for sure, it might be better not to make this change, but just to recommend the use of this ADB on our path in the instructions doc. https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:139: '/paquete/offline_eval_urls.txt' Does this require a corresponding change in the script to put pages at the root instead of under /sdcard ?
lgtm https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:96: adb_path = os.path.join(os.getcwd(), On 2016/12/15 01:46:15, Pete Williamson wrote: > This hardcodes the assumption that we are being run from the chromium/src > directory. Do we already have a limitation that we have to run from there? If > not, it might be good not to introduce a limitation. If so, then this is fine > as is. > > Also, do we know for sure this is the right version of ADB to use? > We could end up with the device updating, and the version of adb under > third_party not getting updated right away, and we might want to be able to > override it. If we don't know for sure, it might be better not to make this > change, but just to recommend the use of this ADB on our path in the > instructions doc. I think this is much more sound - to use the same adb version for push/pull that the test is already wired to use. Perhaps would be good to add a comment to this fact here. https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:139: '/paquete/offline_eval_urls.txt' On 2016/12/15 01:46:15, Pete Williamson wrote: > Does this require a corresponding change in the script to put pages at the root > instead of under /sdcard ? This is just fixing an instance that was missed in previous conversion to use $EXTERNAL_STORAGE path instead of hardcoded /sdcard path.
Made the change to get the adb using relative path. So actually the 'base' path we get is from the build directory (and it's a mandatory argument) and we use relative path for other paths. Pete, PTAnL. Thanks https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:96: adb_path = os.path.join(os.getcwd(), On 2016/12/15 01:46:15, Pete Williamson wrote: > This hardcodes the assumption that we are being run from the chromium/src > directory. Do we already have a limitation that we have to run from there? If > not, it might be good not to introduce a limitation. If so, then this is fine > as is. > > Also, do we know for sure this is the right version of ADB to use? > We could end up with the device updating, and the version of adb under > third_party not getting updated right away, and we might want to be able to > override it. If we don't know for sure, it might be better not to make this > change, but just to recommend the use of this ADB on our path in the > instructions doc. There isn't a limit for the directory where you start the script, as long as you give a correct build directory. And I just made another change to use relative path when looking for adb. So it would not be an issue anymore. As for the version of ADB, I personally would stick to the third_party one *for now* since it's the only one we can make sure that we're using the same version. I'm looking into options to change all adb usage to use catapult/devil in third_party. I think that package is more reliable to use. https://codereview.chromium.org/2576213002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:139: '/paquete/offline_eval_urls.txt' On 2016/12/15 17:50:15, dougarnett wrote: > On 2016/12/15 01:46:15, Pete Williamson wrote: > > Does this require a corresponding change in the script to put pages at the > root > > instead of under /sdcard ? > > This is just fixing an instance that was missed in previous conversion to use > $EXTERNAL_STORAGE path instead of hardcoded /sdcard path. yeah all the path related with the one we use on the device should be fetched from the $EXTERNAL_STORAGE env var on device and this change is fixing a bug that I forgot to use external_dir... and just to clarify it putting pages into 'external_dir' + '/paquete/' not the root directory :)
lgtm
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2576213002/#ps20001 (title: "Change the adb path to relative.")
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": 20001, "attempt_start_ts": 1481836251239220,
"parent_rev": "ad789cc7e483e86bb81defa1ec610f95542ec879", "commit_rev":
"331931a0db2888545fbbb3a6ba0d0dee70d5a267"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Improvements on test harness scripts. Made --use-test-scheduler as default; Ensure adb server is started before copying files and showing config when running the harness. Also fixed a bug where external dir wasn't used. BUG=NONE ========== to ========== [Offline Pages] Improvements on test harness scripts. Made --use-test-scheduler as default; Ensure adb server is started before copying files and showing config when running the harness. Also fixed a bug where external dir wasn't used. BUG=NONE Review-Url: https://codereview.chromium.org/2576213002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Improvements on test harness scripts. Made --use-test-scheduler as default; Ensure adb server is started before copying files and showing config when running the harness. Also fixed a bug where external dir wasn't used. BUG=NONE Review-Url: https://codereview.chromium.org/2576213002 ========== to ========== [Offline Pages] Improvements on test harness scripts. Made --use-test-scheduler as default; Ensure adb server is started before copying files and showing config when running the harness. Also fixed a bug where external dir wasn't used. BUG=NONE Committed: https://crrev.com/0546f6445943373803e79f07b7c99e50909af964 Cr-Commit-Position: refs/heads/master@{#438926} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0546f6445943373803e79f07b7c99e50909af964 Cr-Commit-Position: refs/heads/master@{#438926} |
