Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2157033002:
android: Add tests for ExternalPrerenderHandler. (Closed)
|
Created:
4 years, 5 months ago by Benoit L Modified:
4 years, 5 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionandroid: Add tests for ExternalPrerenderHandler.
Tests used to be there until
https://codereview.chromium.org/2086483004/, where they were
removed (since they weren't testing the right thing), to be re-added
later.
These tests don't run on the UI thread, and check that the prerender
actually starts and complete.
BUG=621919
Committed: https://crrev.com/5b444ab1e097b919006a26ac03659470438d9d65
Cr-Commit-Position: refs/heads/master@{#406889}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comment. #
Total comments: 2
Patch Set 3 : FindBugs. #
Messages
Total messages: 19 (8 generated)
lizeb@chromium.org changed reviewers: + yusufo@chromium.org
lgtm with a single suggestion. Because it is a long file and if I say nothing, it looks as if I lgtm'ed without reading it. https://codereview.chromium.org/2157033002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java (right): https://codereview.chromium.org/2157033002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java:116: }); Should rename addPrerender to private WebContents ensureStartedPrerender(String url) and do all the above?
lizeb@chromium.org changed reviewers: + yfriedman@chromium.org
Done, thanks! +yfriedman as reviewer. https://codereview.chromium.org/2157033002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java (right): https://codereview.chromium.org/2157033002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java:116: }); On 2016/07/21 01:13:33, Yusuf wrote: > Should rename addPrerender to > > private WebContents ensureStartedPrerender(String url) and do all the above? Done.
https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java (right): https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java:96: Thread.sleep(PRERENDER_DELAY_MS); were it not for this, could the tests be @UIThreadTests and avoid the thread-hopping? Or are some of these events async and you need to wait for them to resolve?
https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java (right): https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java:96: Thread.sleep(PRERENDER_DELAY_MS); On 2016/07/21 13:54:14, Yaron wrote: > were it not for this, could the tests be @UIThreadTests and avoid the > thread-hopping? Or are some of these events async and you need to wait for them > to resolve? The previous tests were UIThreadTests. The big issue was that they weren't checking that prerendering had actually started (since it kicks of a lot of async stuff, including on the UI thread). So it's necessary to be on another thread to observe that the page has actually finished prerendering in ensureCompletedPrerenderForUrl().
On 2016/07/21 13:57:30, Benoit L wrote: > https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java > (right): > > https://codereview.chromium.org/2157033002/diff/20001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandlerTest.java:96: > Thread.sleep(PRERENDER_DELAY_MS); > On 2016/07/21 13:54:14, Yaron wrote: > > were it not for this, could the tests be @UIThreadTests and avoid the > > thread-hopping? Or are some of these events async and you need to wait for > them > > to resolve? > > The previous tests were UIThreadTests. The big issue was that they weren't > checking that prerendering had actually started (since it kicks of a lot of > async stuff, including on the UI thread). > > So it's necessary to be on another thread to observe that the page has actually > finished prerendering in ensureCompletedPrerenderForUrl(). right, makes sense. lgtm
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2157033002/#ps20001 (title: "Address comment.")
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
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 lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/2157033002/#ps40001 (title: "FindBugs.")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== android: Add tests for ExternalPrerenderHandler. Tests used to be there until https://codereview.chromium.org/2086483004/, where they were removed (since they weren't testing the right thing), to be re-added later. These tests don't run on the UI thread, and check that the prerender actually starts and complete. BUG=621919 ========== to ========== android: Add tests for ExternalPrerenderHandler. Tests used to be there until https://codereview.chromium.org/2086483004/, where they were removed (since they weren't testing the right thing), to be re-added later. These tests don't run on the UI thread, and check that the prerender actually starts and complete. BUG=621919 Committed: https://crrev.com/5b444ab1e097b919006a26ac03659470438d9d65 Cr-Commit-Position: refs/heads/master@{#406889} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5b444ab1e097b919006a26ac03659470438d9d65 Cr-Commit-Position: refs/heads/master@{#406889} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
