Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432593002:
[Offline Pages] Add basic evaluation tests and related changes. (Closed)
|
Created:
4 years, 2 months ago by romax Modified:
4 years, 1 month ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Add basic evaluation tests and related changes.
This change includes some changes for the basic evaluation tests.
1. Evaluation Java test file.
2. Evaluation scheduler which would only be used in testing and will
start the processing of requests immediately. This will be injected in
request coordinator factory as a testing class.
3. Build files related.
BUG=646947
Committed: https://crrev.com/de5e277b78cc3e4ad3a061ae838ec8b9885ea38b
Cr-Commit-Position: refs/heads/master@{#427913}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments about manual. #
Total comments: 49
Patch Set 3 : Addressed comments. #
Total comments: 6
Patch Set 4 : more comments. #
Total comments: 6
Patch Set 5 : Comments. #Patch Set 6 : Rebased to resolve compile error. #Patch Set 7 : Fix build failure. #Messages
Total messages: 36 (18 generated)
romax@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org, tedchoc@chromium.org
petewil@chromium.org: Please review changes in *offline* tedchoc@chromium.org: Please review changes in chrome/android/ PTAL, Thanks!
Just quick feedback on "manual" terminology https://codereview.chromium.org/2432593002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2432593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:125: * Force request coordinator to process the requests in the queue. doc return val https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java:39: * Manual evaluation tests for OfflinePageBridge.SavePageLater. What is "manual" about this? Really more of a batched set of tests, right? Maybe: * Tests OfflinePageBridge.SavePageLater over a batch of urls. ... ? https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java:44: public class OfflinePageManualEvaluationTest extends ChromeActivityTestCaseBase<ChromeActivity> { OfflinePageSavePageLaterEvaluationTest ? https://codereview.chromium.org/2432593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h (right): https://codereview.chromium.org/2432593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h:58: bool PushRequestProcessing( doc return val - returns if processing started?
Addressed comments about 'manual'. PTAL. Thanks! https://codereview.chromium.org/2432593002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2432593002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:125: * Force request coordinator to process the requests in the queue. On 2016/10/19 16:46:25, dougarnett wrote: > doc return val Done. https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java:39: * Manual evaluation tests for OfflinePageBridge.SavePageLater. On 2016/10/19 16:46:25, dougarnett wrote: > What is "manual" about this? Really more of a batched set of tests, right? > Maybe: > * Tests OfflinePageBridge.SavePageLater over a batch of urls. ... ? Sure... I was thinking this test can only be tested manually so I typed "manual" :) Will fix some comments and naming related. Thanks! https://codereview.chromium.org/2432593002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageManualEvaluationTest.java:44: public class OfflinePageManualEvaluationTest extends ChromeActivityTestCaseBase<ChromeActivity> { On 2016/10/19 16:46:25, dougarnett wrote: > OfflinePageSavePageLaterEvaluationTest ? Done. https://codereview.chromium.org/2432593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h (right): https://codereview.chromium.org/2432593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h:58: bool PushRequestProcessing( On 2016/10/19 16:46:25, dougarnett wrote: > doc return val - returns if processing started? Done.
few more thoughts https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:64: private static final File INPUT_FILE = wonder about just name here in static and then create file for mInputFile below? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:65: new File(Environment.getExternalStorageDirectory(), "paquete/top_em_urls.txt"); "paquete/offline_eval_urls.txt" to match output naming and not dictate what urls they should be? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:205: private void runThroughURLs(List<String> urls) throws InterruptedException, IOException { processURLs() ? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:206: mSem = new Semaphore(0); more purpose-oriented name? mDoneSemaphore ? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:215: if (!mSem.tryAcquire(urls.size() * mTimeoutPerUrlInSeconds, TimeUnit.SECONDS)) { curious - is this timeout value effectively an average per url or is it used on each url as well?
I still have one file left to look at, but here is my feedback so far. https://codereview.chromium.org/2432593002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:49: new OfflinerPolicy(false, true, false, 4, 1, 3600)); All these booleans at the point of call of the constructor are hard to understand while reading the code. Can we replace them with named constants? for instance, const bool kPreferLessTriedPages = true, at point of call, you can use kPreferRetryCountOverRecency or !kPreferRetryCountOverRecency instead of true or false.
https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:40: * Tests against a list of top EM urls, try to call SavePageLater on each of the url. And also grammar nit - And is a conjunction for connecting phrases, not sentences. Might be better as "It also" instead of "And also" https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:46: class TimeDelta { Is there an existing utility class that you can use for this? I'm pretty sure that I've seen time delta classes before somewhere. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:68: private static final int TIMEOUT_MS = 5000; What is this timeout for? 5 seconds is way too small for a page load timeout. A comment or a more explicit name would help. maybe PAGE_MODEL_LOAD_TIMEOUT_MS. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:79: private LongSparseArray<OfflinePageItem> mPages; Why use a LongSparseArray instead of a hash map? I'm not saying it is wrong or worse, I just want to understand, since I normally see maps, but I don't normally see LongSparseArray. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:82: private LongSparseArray<String> mRequestIdToUrl; Why do we have a bunch or arrays instead of just one for each item? Maybe we could have a pure data class (equivalent of a struct) with the information for each URL, and just have one array of those. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:101: return; Should we log or print an error to the console for this error case return? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:126: assertFalse("No valid URLs in the input file.", mUrls.size() == 0); We might add a TODO to get the URL file name from the command line if possible. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:153: protected void setUpBridge(final boolean shortcut) throws InterruptedException { I'd love to see javadoc on these methods. Anything longer than about 3 lines could benefit from some comments telling me the intended purpose of the function (I don't care as much about "@param", especially on private methods, but there might be others who want them). I can tell what a function actually does by reading the code, but I can't always tell what it was intended to do, which can make it difficult to fix in case it doesn't do what it was intended to. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:209: mTimeoutPerUrlInSeconds = 30L; This should be a well named constant at the top of the file, and we should have a brief comment about why we picked the time we did. URL_LOAD_TIMEOUT_SECONDS, perhaps? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:215: if (!mSem.tryAcquire(urls.size() * mTimeoutPerUrlInSeconds, TimeUnit.SECONDS)) { What resource is this semaphore protecting and why? A comment where the semaphore is defined would help. A better name as DougArnett suggests might also be good. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:223: private void getUrls() throws IOException, InterruptedException { maybe getUrlListFromFile() would be a better name, so we don't get confused with fetching the web pages from the URLs. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:289: assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS)); It looks like you may be using the 5 second timeout for several different things, let's have an appropriately named timeout for each place you use it. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:292: private void writeResults(boolean completed) throws IOException, InterruptedException { The javadoc on this could tell me a bit about what it will print - for instance, each line might contain a result code and a URL. This could be real handy in case we write an automated tool to consume the results someday. SUCCESS http://google.com TIMEOUT http://yahoo.com LOAD_FAILURE http://cnn.com https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:297: mOutput.write("Timed out, incompleted results!" + NEW_LINE); Might be good to mention what timed out. Was it a single page? Was it waiting for the offline page model to initialize? Something else? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:326: mTimeoutPerUrlInSeconds = (long) (90); // Timeout for a page is 90 seconds. Use constant. We might need a longer timeout like 3 minutes when on Gin2G poor. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:328: mOutputFile = new File(Environment.getExternalStorageDirectory(), "paquete/results1.txt"); Put this constant at the top of the file. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:335: mTimeoutPerUrlInSeconds = (long) (24 * 60 * 60); // Timeout after 1 day. Ditto about constant.
Comments addressed, PTAL! Thanks https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:40: * Tests against a list of top EM urls, try to call SavePageLater on each of the url. And also On 2016/10/20 17:58:57, Pete Williamson wrote: > grammar nit - And is a conjunction for connecting phrases, not sentences. Might > be better as "It also" instead of "And also" Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:46: class TimeDelta { On 2016/10/20 17:58:57, Pete Williamson wrote: > Is there an existing utility class that you can use for this? I'm pretty sure > that I've seen time delta classes before somewhere. from Chromium code search i've seen a C++ TimeDelta class but not a Java one. Should I change to a class that comes with Java (like Java.time)? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:64: private static final File INPUT_FILE = On 2016/10/19 23:20:33, dougarnett wrote: > wonder about just name here in static and then create file for mInputFile below? > Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:65: new File(Environment.getExternalStorageDirectory(), "paquete/top_em_urls.txt"); On 2016/10/19 23:20:33, dougarnett wrote: > "paquete/offline_eval_urls.txt" to match output naming and not dictate what urls > they should be? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:68: private static final int TIMEOUT_MS = 5000; On 2016/10/20 17:58:57, Pete Williamson wrote: > What is this timeout for? 5 seconds is way too small for a page load timeout. > A comment or a more explicit name would help. maybe PAGE_MODEL_LOAD_TIMEOUT_MS. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:79: private LongSparseArray<OfflinePageItem> mPages; On 2016/10/20 17:58:58, Pete Williamson wrote: > Why use a LongSparseArray instead of a hash map? I'm not saying it is wrong or > worse, I just want to understand, since I normally see maps, but I don't > normally see LongSparseArray. It's suggested by Android Lint and I think it is more memory-efficient. And it was suggested probably because the key type of the map was Long. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:82: private LongSparseArray<String> mRequestIdToUrl; On 2016/10/20 17:58:57, Pete Williamson wrote: > Why do we have a bunch or arrays instead of just one for each item? Maybe we > could have a pure data class (equivalent of a struct) with the information for > each URL, and just have one array of those. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:101: return; On 2016/10/20 17:58:57, Pete Williamson wrote: > Should we log or print an error to the console for this error case return? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:126: assertFalse("No valid URLs in the input file.", mUrls.size() == 0); On 2016/10/20 17:58:57, Pete Williamson wrote: > We might add a TODO to get the URL file name from the command line if possible. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:153: protected void setUpBridge(final boolean shortcut) throws InterruptedException { On 2016/10/20 17:58:57, Pete Williamson wrote: > I'd love to see javadoc on these methods. Anything longer than about 3 lines > could benefit from some comments telling me the intended purpose of the function > (I don't care as much about "@param", especially on private methods, but there > might be others who want them). I can tell what a function actually does by > reading the code, but I can't always tell what it was intended to do, which can > make it difficult to fix in case it doesn't do what it was intended to. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:205: private void runThroughURLs(List<String> urls) throws InterruptedException, IOException { On 2016/10/19 23:20:33, dougarnett wrote: > processURLs() ? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:206: mSem = new Semaphore(0); On 2016/10/19 23:20:33, dougarnett wrote: > more purpose-oriented name? mDoneSemaphore ? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:209: mTimeoutPerUrlInSeconds = 30L; On 2016/10/20 17:58:57, Pete Williamson wrote: > This should be a well named constant at the top of the file, and we should have > a brief comment about why we picked the time we did. URL_LOAD_TIMEOUT_SECONDS, > perhaps? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:215: if (!mSem.tryAcquire(urls.size() * mTimeoutPerUrlInSeconds, TimeUnit.SECONDS)) { On 2016/10/19 23:20:33, dougarnett wrote: > curious - is this timeout value effectively an average per url or is it used on > each url as well? Fixed the semaphore naming part. For the timeout value: This is *supposed* to be used as a value for each url, but the way i'm using it is more likely the average. So basically the *total* processing time of X urls would be X*mTimeoutPerUrlInSeconds, where there's no timeout limit for each URL. I had no issue but maybe it was because I was on at least GIN-3G. Maybe increasing this default value to ~2minutes would be better? Any suggestions? https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:223: private void getUrls() throws IOException, InterruptedException { On 2016/10/20 17:58:57, Pete Williamson wrote: > maybe getUrlListFromFile() would be a better name, so we don't get confused with > fetching the web pages from the URLs. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:289: assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS)); On 2016/10/20 17:58:57, Pete Williamson wrote: > It looks like you may be using the 5 second timeout for several different > things, let's have an appropriately named timeout for each place you use it. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:292: private void writeResults(boolean completed) throws IOException, InterruptedException { On 2016/10/20 17:58:57, Pete Williamson wrote: > The javadoc on this could tell me a bit about what it will print - for instance, > each line might contain a result code and a URL. This could be real handy in > case we write an automated tool to consume the results someday. > > SUCCESS http://google.com > TIMEOUT http://yahoo.com > LOAD_FAILURE http://cnn.com Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:297: mOutput.write("Timed out, incompleted results!" + NEW_LINE); On 2016/10/20 17:58:57, Pete Williamson wrote: > Might be good to mention what timed out. Was it a single page? Was it waiting > for the offline page model to initialize? Something else? Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:326: mTimeoutPerUrlInSeconds = (long) (90); // Timeout for a page is 90 seconds. On 2016/10/20 17:58:57, Pete Williamson wrote: > Use constant. We might need a longer timeout like 3 minutes when on Gin2G poor. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:328: mOutputFile = new File(Environment.getExternalStorageDirectory(), "paquete/results1.txt"); On 2016/10/20 17:58:57, Pete Williamson wrote: > Put this constant at the top of the file. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:335: mTimeoutPerUrlInSeconds = (long) (24 * 60 * 60); // Timeout after 1 day. On 2016/10/20 17:58:57, Pete Williamson wrote: > Ditto about constant. Done. https://codereview.chromium.org/2432593002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:49: new OfflinerPolicy(false, true, false, 4, 1, 3600)); On 2016/10/20 00:46:28, Pete Williamson wrote: > All these booleans at the point of call of the constructor are hard to > understand while reading the code. Can we replace them with named constants? > for instance, > const bool kPreferLessTriedPages = true, > > at point of call, you can use kPreferRetryCountOverRecency or > !kPreferRetryCountOverRecency instead of true or false. Done.
https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:46: class TimeDelta { On 2016/10/22 02:08:55, romax wrote: > On 2016/10/20 17:58:57, Pete Williamson wrote: > > Is there an existing utility class that you can use for this? I'm pretty sure > > that I've seen time delta classes before somewhere. > > from Chromium code search i've seen a C++ TimeDelta class but not a Java one. > Should I change to a class that comes with Java (like Java.time)? Unless it makes your code a lot more complicated, it's likely better to change to a Java.time class, if it meets your needs. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:215: if (!mSem.tryAcquire(urls.size() * mTimeoutPerUrlInSeconds, TimeUnit.SECONDS)) { On 2016/10/22 02:08:54, romax wrote: > On 2016/10/19 23:20:33, dougarnett wrote: > > curious - is this timeout value effectively an average per url or is it used > on > > each url as well? > > Fixed the semaphore naming part. > For the timeout value: > This is *supposed* to be used as a value for each url, but the way i'm using it > is more likely the average. So basically the *total* processing time of X urls > would be X*mTimeoutPerUrlInSeconds, where there's no timeout limit for each URL. > I had no issue but maybe it was because I was on at least GIN-3G. Maybe > increasing this default value to ~2minutes would be better? Any suggestions? Yes, 2 minutes would be better on Gin2G, 3 minutes would be better on Gin2G-poor. It would be awesome if the code could tell the difference, but if not, using the largest of these is probably a good idea. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:115: NotificationManager nm = As I recall, our code style guides say to prefer spelling it out: notificationManager instead of nm. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:147: private void logError(String error) { Why not use the existing log methods? import org.chromium.base.Log; ... Log.d(TAG, "@@@@@@ "); See BackgroundOfflinerTask.java for an example of its use. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:427: public void testFailureRate() throws IOException, InterruptedException { Is failure rate ever calculated and printed somewhere? The test name implies that it is, but I didn't see a failure rate caluclation in either processUrls or writeResults.
romax@chromium.org changed reviewers: - tedchoc@chromium.org
Addressed comments, PTAL, thanks! https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:46: class TimeDelta { On 2016/10/24 20:48:22, Pete Williamson wrote: > On 2016/10/22 02:08:55, romax wrote: > > On 2016/10/20 17:58:57, Pete Williamson wrote: > > > Is there an existing utility class that you can use for this? I'm pretty > sure > > > that I've seen time delta classes before somewhere. > > > > from Chromium code search i've seen a C++ TimeDelta class but not a Java one. > > Should I change to a class that comes with Java (like Java.time)? > > Unless it makes your code a lot more complicated, it's likely better to change > to a Java.time class, if it meets your needs. Just did a broader code search, seems like most of code is using long as timestamps/durations. so I'm keeping this. https://codereview.chromium.org/2432593002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:215: if (!mSem.tryAcquire(urls.size() * mTimeoutPerUrlInSeconds, TimeUnit.SECONDS)) { On 2016/10/24 20:48:22, Pete Williamson wrote: > On 2016/10/22 02:08:54, romax wrote: > > On 2016/10/19 23:20:33, dougarnett wrote: > > > curious - is this timeout value effectively an average per url or is it used > > on > > > each url as well? > > > > Fixed the semaphore naming part. > > For the timeout value: > > This is *supposed* to be used as a value for each url, but the way i'm using > it > > is more likely the average. So basically the *total* processing time of X urls > > would be X*mTimeoutPerUrlInSeconds, where there's no timeout limit for each > URL. > > I had no issue but maybe it was because I was on at least GIN-3G. Maybe > > increasing this default value to ~2minutes would be better? Any suggestions? > > Yes, 2 minutes would be better on Gin2G, 3 minutes would be better on > Gin2G-poor. It would be awesome if the code could tell the difference, but if > not, using the largest of these is probably a good idea. Done. Added a TODO for differentiating network conditions. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:115: NotificationManager nm = On 2016/10/24 20:48:22, Pete Williamson wrote: > As I recall, our code style guides say to prefer spelling it out: > notificationManager instead of nm. Done. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:147: private void logError(String error) { On 2016/10/24 20:48:22, Pete Williamson wrote: > Why not use the existing log methods? > import org.chromium.base.Log; > ... > Log.d(TAG, "@@@@@@ "); > > See BackgroundOfflinerTask.java for an example of its use. I was thinking if it was better to log the error message both on console and file. So i'm using this method as a wrapper. https://codereview.chromium.org/2432593002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:427: public void testFailureRate() throws IOException, InterruptedException { On 2016/10/24 20:48:22, Pete Williamson wrote: > Is failure rate ever calculated and printed somewhere? The test name implies > that it is, but I didn't see a failure rate caluclation in either processUrls or > writeResults. Done.
lgtm
romax@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil@chromium.org: Please review changes in chrome/android/javatests Thanks!
chrome/android/javatests lgtm with some small nits. https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:92: private boolean mUseTestScheduler; Looks like this could just be a local variable in each test? https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:265: boolean res = BackgroundSchedulerBridge.startProcessing( nit: |res| is unused? https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:342: return "NOT COMPLETED"; Did you mean to use underscores here and below?
lgtm
Addressed comments from Sami. Going to do a dry run before committing. https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java (right): https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:92: private boolean mUseTestScheduler; On 2016/10/26 12:23:24, Sami wrote: > Looks like this could just be a local variable in each test? Yes :) But i'm keeping them separately because the next step(soon) would be pull these 'parameters' out into a config/option of a runner script. it's also the same for mIsUserRequested and several timeout arguments. https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:265: boolean res = BackgroundSchedulerBridge.startProcessing( On 2016/10/26 12:23:24, Sami wrote: > nit: |res| is unused? Done. Just realized that this whole method was unused. Removed. https://codereview.chromium.org/2432593002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java:342: return "NOT COMPLETED"; On 2016/10/26 12:23:24, Sami wrote: > Did you mean to use underscores here and below? Done.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2432593002/#ps120001 (title: "Fix build failure.")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Add basic evaluation tests and related changes. This change includes some changes for the basic evaluation tests. 1. Evaluation Java test file. 2. Evaluation scheduler which would only be used in testing and will start the processing of requests immediately. This will be injected in request coordinator factory as a testing class. 3. Build files related. BUG=646947 ========== to ========== [Offline Pages] Add basic evaluation tests and related changes. This change includes some changes for the basic evaluation tests. 1. Evaluation Java test file. 2. Evaluation scheduler which would only be used in testing and will start the processing of requests immediately. This will be injected in request coordinator factory as a testing class. 3. Build files related. BUG=646947 Committed: https://crrev.com/de5e277b78cc3e4ad3a061ae838ec8b9885ea38b Cr-Commit-Position: refs/heads/master@{#427913} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/de5e277b78cc3e4ad3a061ae838ec8b9885ea38b Cr-Commit-Position: refs/heads/master@{#427913}
Message was sent while issue was closed.
On 2016/10/27 02:09:16, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/de5e277b78cc3e4ad3a061ae838ec8b9885ea38b > Cr-Commit-Position: refs/heads/master@{#427913} Just ran into this when doing an official build locally: ../../chrome/browser/android/offline_pages/request_coordinator_factory.cc:38:12: error: unused variable 'kPreferUntriedRequest' [-Werror,-Wunused-const-variable] const bool kPreferUntriedRequest = false; ^ Looks like the constants might need to be wrapped in "#if !defined(OFFICIAL_BUILD)" as well. I'm not actually sure if any bots will catch this, I don't think any of the release builders use clang. |
