Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655273003:
Adds a first integration test for the Recent Tabs feature. (Closed)
|
Created:
3 years, 11 months ago by dewittj Modified:
3 years, 9 months 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, Pete Williamson, dougarnett Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a first integration test for the Recent Tabs feature.
The test simply loads a tab, waits for the snapshot timeout, and then creates a
new tab, verifying that the snapshot for the first tab exists.
Also:
* adds a command-line flag that reduces timeouts.
* Stops using size_t for timeout values, instead using the native
int64_t that TimeDelta uses. size_t can be 32 or 64 bits depending
on platform, so it sometimes will need a cast to int64_t. Using
the same type of the API prevents subtle platform issues.
BUG=
Review-Url: https://codereview.chromium.org/2655273003
Cr-Commit-Position: refs/heads/master@{#453338}
Committed: https://chromium.googlesource.com/chromium/src/+/50b7420693312c97fe1e3f6ad0a7c7272eb930c0
Patch Set 1 #
Total comments: 18
Patch Set 2 : Comments addressed. #Patch Set 3 : Rename test and move around command line flags for readability. #Patch Set 4 : Change type to be happy on all platforms #Patch Set 5 : Fix wrapping of comment text. #
Total comments: 6
Messages
Total messages: 35 (24 generated)
Description was changed from ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also adds a command-line flag that reduces timeouts. BUG= ========== to ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also adds a command-line flag that reduces timeouts. BUG= ==========
dewittj@chromium.org changed reviewers: + carlosk@chromium.org, dimich@chromium.org
Description was changed from ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also adds a command-line flag that reduces timeouts. BUG= ========== to ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also: * adds a command-line flag that reduces timeouts. * Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. BUG= ==========
carlosk, dimich: PTAL petewil, dougarnett: CC'd FYI - since this affects SnapshotController.
LGTM as longs at the unused constants are addressed. https://codereview.chromium.org/2655273003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:29: public static final String LAST_N_NAMESPACE = "last_n"; nit: could a comment be added here notifying that these values must be kept in sync with what's defined in client_namespace_constants.(h|cc)? And possibly a mirror comment there too. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:32: private static final ClientId LAST_N_ID = Both POLLING_INTERVAL and LAST_N_ID are not being used. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:111: Thread.sleep(20); Sleeping in tests always makes me cringe but I don't see any way around it while our own RMD strategy involves sleeping... :( https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.h:70: SnapshotController::Client* client); nit: why the constructor order change? Isn't this just a change in argument type?
https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:111: Thread.sleep(20); On 2017/01/27 02:28:14, carlosk wrote: > Sleeping in tests always makes me cringe but I don't see any way around it while > our own RMD strategy involves sleeping... :( Yes, is it possible to wait for a notification or observe something here? https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.cc:42: if (offline_pages::ShouldUseTestingSnapshotDelay()) { Instead of adding a flag modifying behavior for test, could the test retrieve the tab, then RecentTabHelper and use the RecentTabHelper::SetDelegate() to set a delegate that provides a tast version of task_runner which can be advanced instantly? This can be part of OfflinePageBridge..
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
* Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. I am guessing there is a good reason, but could you explain it? https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:102: @MediumTest https://developer.android.com/reference/android/support/test/filters/MediumTe... vs. https://developer.android.com/reference/android/support/test/filters/LargeTes... https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:103: public void testLastNPageSavedWhenTabSwitchedAfterLoadAndDelay() throws Exception { Can you add a bit of documentation to this test? (And simplify the name) https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:111: Thread.sleep(20); On 2017/01/27 03:10:25, Dmitry Titov wrote: > On 2017/01/27 02:28:14, carlosk wrote: > > Sleeping in tests always makes me cringe but I don't see any way around it > while > > our own RMD strategy involves sleeping... :( > > Yes, is it possible to wait for a notification or observe something here? do we inform NTP of new last_n pages? https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:115: nit: You can probably move the clientId line here, as it will not be used until this place.
The CQ bit was checked by dewittj@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...
Description was changed from ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also: * adds a command-line flag that reduces timeouts. * Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. BUG= ========== to ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also: * adds a command-line flag that reduces timeouts. * Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. size_t can be 32 or 64 bits depending on platform, so it sometimes will need a cast to int64_t. Using the same type of the API prevents subtle platform issues. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2655273003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:29: public static final String LAST_N_NAMESPACE = "last_n"; On 2017/01/27 02:28:14, carlosk wrote: > nit: could a comment be added here notifying that these values must be kept in > sync with what's defined in client_namespace_constants.(h|cc)? And possibly a > mirror comment there too. Done. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:32: private static final ClientId LAST_N_ID = On 2017/01/27 02:28:14, carlosk wrote: > Both POLLING_INTERVAL and LAST_N_ID are not being used. Done. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:102: @MediumTest On 2017/01/27 17:47:13, fgorski wrote: > https://developer.android.com/reference/android/support/test/filters/MediumTe... > vs. > https://developer.android.com/reference/android/support/test/filters/LargeTes... Now that sleep is removed, I think it should run in <1s so MediumTest is appropriate. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:103: public void testLastNPageSavedWhenTabSwitchedAfterLoadAndDelay() throws Exception { On 2017/01/27 17:47:13, fgorski wrote: > Can you add a bit of documentation to this test? (And simplify the name) Added more docs within the test, and simplified the name. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:111: Thread.sleep(20); To reply to everyone :) The issue here was not to wait for a new offline page, I needed to wait until SnapshotController believed it was in a good enough state to actually generate an offline page /after/ switching tabs away. I worked around this sleep by making the delays in test 0ms, which means that the page will always be ready to snapshot at the moment DomContentLoaded fires. This test waits for onload, which should always happen after DCL. https://codereview.chromium.org/2655273003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:115: On 2017/01/27 17:47:13, fgorski wrote: > nit: You can probably move the clientId line here, as it will not be used until > this place. Acknowledged. https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.cc:42: if (offline_pages::ShouldUseTestingSnapshotDelay()) { On 2017/01/27 03:10:25, Dmitry Titov wrote: > Instead of adding a flag modifying behavior for test, could the test retrieve > the tab, then RecentTabHelper and use the RecentTabHelper::SetDelegate() to set > a delegate that provides a tast version of task_runner which can be advanced > instantly? This can be part of OfflinePageBridge.. That sounds like a lot of plumbing for this test. Also not sure if it's appropriate to have a mock-clock type of time in just one component of the browser. We discussed the need for the quality framework to observe snapshot conroller, and at that point we will have a more reliable signal and could remove this flag. Does that seem ok to you? https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2655273003/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.h:70: SnapshotController::Client* client); On 2017/01/27 02:28:14, carlosk wrote: > nit: why the constructor order change? Isn't this just a change in argument > type? Done.
The CQ bit was checked by dewittj@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dewittj@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 checked by dewittj@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.
Bumping this one. Is there a reason why this wasn't landed? I am considering adding a new integration test for last_n and the harness created here could be useful.
lgtm
lgtm with comments on threading related calls. https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:38: ThreadUtils.runOnUiThread(new Runnable() { did you try: runOnUiThreadBlocking with no semaphore as you did with NCN below? https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:122: ThreadUtils.runOnUiThread(new Runnable() { did you try: runOnUiThreadBlocking with no semaphore as you did with NCN above? https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:145: ThreadUtils.runOnUiThread(new Runnable() { Does this work? OfflinePageItem item = ThreadUtils.runOnUiThreadBlockingNoException(new Callable<OfflinePageItem>() { @Override public OfflinePageItem call() throws Exception { ... } }
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:38: ThreadUtils.runOnUiThread(new Runnable() { On 2017/02/23 20:23:12, fgorski wrote: > did you try: runOnUiThreadBlocking with no semaphore as you did with NCN below? This does not work because we need to wait for the async Loaded event. The NCN API is synchronous so no semaphore is required. https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:122: ThreadUtils.runOnUiThread(new Runnable() { On 2017/02/23 20:23:11, fgorski wrote: > did you try: runOnUiThreadBlocking with no semaphore as you did with NCN above? ditto. https://codereview.chromium.org/2655273003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:145: ThreadUtils.runOnUiThread(new Runnable() { On 2017/02/23 20:23:12, fgorski wrote: > Does this work? > OfflinePageItem item = ThreadUtils.runOnUiThreadBlockingNoException(new > Callable<OfflinePageItem>() { > @Override public OfflinePageItem call() throws Exception { > ... > } > } > No, and for the same reason. Since the API is async, the Blocking methods (and really any of the ThreadUtils functions that use a Callable) won't be suitable.
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/2655273003/#ps80001 (title: "Fix wrapping of comment text.")
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": 80001, "attempt_start_ts": 1488225232608850,
"parent_rev": "1b0bd13a33108285880022789c626f1b8d08dabd", "commit_rev":
"50b7420693312c97fe1e3f6ad0a7c7272eb930c0"}
Message was sent while issue was closed.
Description was changed from ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also: * adds a command-line flag that reduces timeouts. * Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. size_t can be 32 or 64 bits depending on platform, so it sometimes will need a cast to int64_t. Using the same type of the API prevents subtle platform issues. BUG= ========== to ========== Adds a first integration test for the Recent Tabs feature. The test simply loads a tab, waits for the snapshot timeout, and then creates a new tab, verifying that the snapshot for the first tab exists. Also: * adds a command-line flag that reduces timeouts. * Stops using size_t for timeout values, instead using the native int64_t that TimeDelta uses. size_t can be 32 or 64 bits depending on platform, so it sometimes will need a cast to int64_t. Using the same type of the API prevents subtle platform issues. BUG= Review-Url: https://codereview.chromium.org/2655273003 Cr-Commit-Position: refs/heads/master@{#453338} Committed: https://chromium.googlesource.com/chromium/src/+/50b7420693312c97fe1e3f6ad0a7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/50b7420693312c97fe1e3f6ad0a7... |
