Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493673003:
[Offline Pages] Root access no more required by test harness. (Closed)
|
Created:
4 years, 1 month ago by romax Modified:
4 years, 1 month ago Reviewers:
Pete Williamson 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] Root access no more required by test harness.
Making root access unnecessary by adding a step which would copy saved
offline pages to external storage after test finishes. So that unrooted
devices can run the test harness script.
Also added an option to select which device to use when more than one
devices are connected.
BUG=661395
Committed: https://crrev.com/82b22f4df42550e6614719076463ceea50dcfa09
Cr-Commit-Position: refs/heads/master@{#431739}
Patch Set 1 #
Total comments: 5
Patch Set 2 : comment. #Patch Set 3 : Fix build. #Patch Set 4 : fix more #Patch Set 5 : fix #Patch Set 6 : fix #
Total comments: 1
Messages
Total messages: 27 (18 generated)
Description was changed from ========== [Offline Pages] Root access not more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. BUG=661395 ========== to ========== [Offline Pages] Root access not more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ==========
romax@chromium.org changed reviewers: + petewil@chromium.org
PTAL, thanks! Tried locally and it worked.
Description was changed from ========== [Offline Pages] Root access not more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ========== to ========== [Offline Pages] Root access no more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ==========
https://codereview.chromium.org/2493673003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2493673003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:151: // Remove the old archive folder. Why remove the old folder? Maybe we should just empty it, and then use the existing directory. https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:78: parser.add_argument( Why do we need this? I think that normal practice is just to set the ANDROID_SERIAL environment variable to the device you want ADB to use.
Addressed comments, PTAL! Thanks! https://codereview.chromium.org/2493673003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2493673003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:151: // Remove the old archive folder. On 2016/11/10 02:00:33, Pete Williamson wrote: > Why remove the old folder? Maybe we should just empty it, and then use the > existing directory. Done. https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:78: parser.add_argument( On 2016/11/10 02:00:33, Pete Williamson wrote: > Why do we need this? I think that normal practice is just to set the > ANDROID_SERIAL environment variable to the device you want ADB to use. I was thinking about the use case that you can have >1 devices connected to local machine and they can run with different options/networks etc. And the previous script would not support this case. It's totally fine if it's not a valid use case I'll remove this, it's just a quick thought :)
lgtm https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2493673003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:78: parser.add_argument( On 2016/11/10 02:45:55, romax wrote: > On 2016/11/10 02:00:33, Pete Williamson wrote: > > Why do we need this? I think that normal practice is just to set the > > ANDROID_SERIAL environment variable to the device you want ADB to use. > > I was thinking about the use case that you can have >1 devices connected to > local machine and they can run with different options/networks etc. And the > previous script would not support this case. > It's totally fine if it's not a valid use case I'll remove this, it's just a > quick thought :) It should be OK to leave it in.
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ran into build errors.. made some minor changes, PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with question. https://codereview.chromium.org/2493673003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2493673003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:430: // Move the page to external storage if external archive exists. Should we create the external storage folder if it doesn't exist, or does the script do that?
The CQ bit was checked by romax@chromium.org
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 ========== [Offline Pages] Root access no more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ========== to ========== [Offline Pages] Root access no more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Root access no more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 ========== to ========== [Offline Pages] Root access no more required by test harness. Making root access unnecessary by adding a step which would copy saved offline pages to external storage after test finishes. So that unrooted devices can run the test harness script. Also added an option to select which device to use when more than one devices are connected. BUG=661395 Committed: https://crrev.com/82b22f4df42550e6614719076463ceea50dcfa09 Cr-Commit-Position: refs/heads/master@{#431739} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/82b22f4df42550e6614719076463ceea50dcfa09 Cr-Commit-Position: refs/heads/master@{#431739} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
