Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465303003:
[Offline Pages] Script for running evaluation tests. (Closed)
|
Created:
4 years, 1 month ago by romax Modified:
4 years, 1 month 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] Script for running evaluation tests.
Added the functionality to the evaluation test to parse config file and
read the parameters. Also have a script which would allow user to run
the test harness more easily.
BUG=661395
Committed: https://crrev.com/4f3c7214424f1dab63e6d773feaf371457d7fafd
Cr-Commit-Position: refs/heads/master@{#429996}
Patch Set 1 #
Total comments: 21
Patch Set 2 : comments. #
Total comments: 2
Patch Set 3 : more comments. #
Total comments: 4
Patch Set 4 : fix build. #
Messages
Total messages: 26 (12 generated)
romax@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:16: DEFAULT_URL_TIMEOUT = 180 too small for 2g, consider upto 8 minutes instead (or we can split difference for default at 5 or 6 minutes)
https://codereview.chromium.org/2465303003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2465303003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:438: * which would provide simplicity to run this test. Nit - this line of your comment is a bit unclear. Did you mean there is a script which can be simplified by running this test? there is a script which can make the user's experience simpler by utilizing this test? this is complicated, but using a script can make the call simpler? https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:8: # This script is used to run tests for SavePageLater evaluation. Are the instructions in "Instructions to run OfflinePageSavePageLaterEvaluationTest" updated to show how to use the script? https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:16: DEFAULT_URL_TIMEOUT = 180 On 2016/11/02 19:34:15, dougarnett wrote: > too small for 2g, consider upto 8 minutes instead (or we can split difference > for default at 5 or 6 minutes) Should there be a way to use the current timeout policy of the request coordinator instead of overriding it? Our (soon to be current) timeout policy is 3 tries, and tries are 8 minutes for an "immediate" try, 3 minutes for a "scheduled" try. (so we might allow as much as 24 minutes for an "immediate" try, and 9 minutes for a scheduled try). https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:26: Maybe you could provice a bit of guidance here in how to use the script. An example invocation with the flags most people will need would be good. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:34: help='Directory for output, default is ~/offline_eval_output/') Thanks for adding the help! https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:39: help='Time out per url, in seconds.') Maybe mention default here, and in other help items? https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:44: help='Test as user-requested urls.') --user-request should default to true, does it? (I see DEFAULT_USER_REQUEST=False further up in the file) https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:97: '/sdcard/paquete/offline_eval_urls.txt' What happens on a device with no SD card, like the nexus?
Addressed comments, PTAL! Thanks! https://codereview.chromium.org/2465303003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2465303003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:438: * which would provide simplicity to run this test. On 2016/11/02 20:30:29, Pete Williamson wrote: > Nit - this line of your comment is a bit unclear. Did you mean > > there is a script which can be simplified by running this test? > > there is a script which can make the user's experience simpler by utilizing this > test? > > this is complicated, but using a script can make the call simpler? I think I meant using the script would be easier than running the test directly... updated the comments. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:8: # This script is used to run tests for SavePageLater evaluation. On 2016/11/02 20:30:29, Pete Williamson wrote: > Are the instructions in "Instructions to run > OfflinePageSavePageLaterEvaluationTest" updated to show how to use the script? Not yet, I'm planning to update the instructions after the patch lands. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:16: DEFAULT_URL_TIMEOUT = 180 On 2016/11/02 19:34:15, dougarnett wrote: > too small for 2g, consider upto 8 minutes instead (or we can split difference > for default at 5 or 6 minutes) Changed default to 480sec. I think it makes sense to use policy to control the test time limit when using test scheduler. However when it comes to test with actual scheduling, I prefer to have a explicit time limit for the test. I'm marking it as TODO. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:26: On 2016/11/02 20:30:29, Pete Williamson wrote: > Maybe you could provice a bit of guidance here in how to use the script. An > example invocation with the flags most people will need would be good. Should I attach the link to the instructions? But an example has been added at the top of the script. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:34: help='Directory for output, default is ~/offline_eval_output/') On 2016/11/02 20:30:29, Pete Williamson wrote: > Thanks for adding the help! :) https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:39: help='Time out per url, in seconds.') On 2016/11/02 20:30:29, Pete Williamson wrote: > Maybe mention default here, and in other help items? Done. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:44: help='Test as user-requested urls.') On 2016/11/02 20:30:29, Pete Williamson wrote: > --user-request should default to true, does it? (I see > DEFAULT_USER_REQUEST=False further up in the file) Done. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:97: '/sdcard/paquete/offline_eval_urls.txt' On 2016/11/02 20:30:29, Pete Williamson wrote: > What happens on a device with no SD card, like the nexus? the 'virtual' external storage would be mounted to /sdcard/ on nexus 7, (but i'm not sure on other devices). However I found an 'adb' way to get the external storage.
https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:26: On 2016/11/02 20:30:29, Pete Williamson wrote: > Maybe you could provice a bit of guidance here in how to use the script. An > example invocation with the flags most people will need would be good. It is better to have instructions here, since Chromium is open source, and coders won't be able to look at private docs. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:97: '/sdcard/paquete/offline_eval_urls.txt' On 2016/11/03 01:46:50, romax wrote: > On 2016/11/02 20:30:29, Pete Williamson wrote: > > What happens on a device with no SD card, like the nexus? > > the 'virtual' external storage would be mounted to /sdcard/ on nexus 7, (but i'm > not sure on other devices). However I found an 'adb' way to get the external > storage. So will this work on my Nexus 5X and Nexus 6 devices which don't have an SD card? https://codereview.chromium.org/2465303003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:45: '--not-user-requested', It is generally better to phrase options in the positive - "user-requested" is always better than "not-user-requested". It will make your code simpler to understand.
updated per Pete's comments, PTAL! https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:26: On 2016/11/03 18:21:11, Pete Williamson wrote: > On 2016/11/02 20:30:29, Pete Williamson wrote: > > Maybe you could provice a bit of guidance here in how to use the script. An > > example invocation with the flags most people will need would be good. > > It is better to have instructions here, since Chromium is open source, and > coders won't be able to look at private docs. Done. https://codereview.chromium.org/2465303003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:97: '/sdcard/paquete/offline_eval_urls.txt' On 2016/11/03 18:21:11, Pete Williamson wrote: > On 2016/11/03 01:46:50, romax wrote: > > On 2016/11/02 20:30:29, Pete Williamson wrote: > > > What happens on a device with no SD card, like the nexus? > > > > the 'virtual' external storage would be mounted to /sdcard/ on nexus 7, (but > i'm > > not sure on other devices). However I found an 'adb' way to get the external > > storage. > > So will this work on my Nexus 5X and Nexus 6 devices which don't have an SD > card? Done. Per discussion it should work, but I'd like to get one and try :) https://codereview.chromium.org/2465303003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:45: '--not-user-requested', On 2016/11/03 18:21:11, Pete Williamson wrote: > It is generally better to phrase options in the positive - "user-requested" is > always better than "not-user-requested". It will make your code simpler to > understand. after some self-struggling i'm adding all options(user_requested, not_user_requested, use_test_scheduler, not_use_test_scheduler)... this will make me feel better...
lgtm
https://codereview.chromium.org/2465303003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2465303003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:96: // TODO(romax): Use actual policy to determine the timeout. Btw, actually shouldn't be too hard to do as the default. Create new OfflinerPolicy() and then get from GetSinglePageTimeLimitForImmediateLoadInSeconds() https://codereview.chromium.org/2465303003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:154: nit - extra blank line
Looks like I forgot the lgtm with my last message just with a nit.
replied to nits. ready to commit. https://codereview.chromium.org/2465303003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2465303003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:96: // TODO(romax): Use actual policy to determine the timeout. On 2016/11/04 00:08:12, dougarnett wrote: > Btw, actually shouldn't be too hard to do as the default. > Create new OfflinerPolicy() and then get from > GetSinglePageTimeLimitForImmediateLoadInSeconds() Great! Will do in future patch. https://codereview.chromium.org/2465303003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py (right): https://codereview.chromium.org/2465303003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py:154: On 2016/11/04 00:08:12, dougarnett wrote: > nit - extra blank line I used FormatCode in vim which applies Google Python styles I guess... and some other scripts also have 2 blank lines here(https://cs.chromium.org/chromium/src/build/android/test_runner.py?rcl=0&l=960) So I'm leaving it as is. Thanks!
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: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2465303003/#ps60001 (title: "fix build.")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Script for running evaluation tests. Added the functionality to the evaluation test to parse config file and read the parameters. Also have a script which would allow user to run the test harness more easily. BUG=661395 ========== to ========== [Offline Pages] Script for running evaluation tests. Added the functionality to the evaluation test to parse config file and read the parameters. Also have a script which would allow user to run the test harness more easily. BUG=661395 Committed: https://crrev.com/4f3c7214424f1dab63e6d773feaf371457d7fafd Cr-Commit-Position: refs/heads/master@{#429996} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4f3c7214424f1dab63e6d773feaf371457d7fafd Cr-Commit-Position: refs/heads/master@{#429996} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
