Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833653002:
Last_n: instrumentation tests to verify timely snapshot deletion. (Closed)
DescriptionLast_n: instrumentation tests to verify timely snapshot deletion.
Adds new tests to RecentTabsTest.java to verify that offline snapshots created
by last_n are properly deleted when successfully navigating to another page and
upon tab closure. These actions are important to comply with privacy
requirements for this feature.
This change re-adds instrumentation tests previously committed along with a
related bugfix [1] that were reverted due to them being flaky. It seems that the
underlying issue causing the flakyness has been fixed [2] [3] so these should
now work as expected.
[1] https://codereview.chromium.org/2824623002/
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted)
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490
BUG=711770
Review-Url: https://codereview.chromium.org/2833653002
Cr-Commit-Position: refs/heads/master@{#487727}
Committed: https://chromium.googlesource.com/chromium/src/+/f6fd25d5826f1a62010b08037eb50c396b05d7a2
Patch Set 1 : Original instrumentation tests from https://codereview.chromium.org/2824623002/ #Patch Set 2 : Rebase. #Patch Set 3 : Rebase #Patch Set 4 : Rebase. #Patch Set 5 : Yet one more rebase #
Total comments: 16
Patch Set 6 : Address CR comments. #Patch Set 7 : Simplified page deletion monitoring #
Messages
Total messages: 33 (24 generated)
The CQ bit was checked by carlosk@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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_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 carlosk@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 carlosk@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Last_n: instrumentation tests to confirm snapshots are properly deleted. Note: this is a partial re-land of https://codereview.chromium.org/2824623002/, containing only the (now fixed) instrumentation tests that were found to be flaky. A privacy concern when offlining recent pages is to delete snapshots on two specific events: navigation to a new page and tab closure. This change adds two new instrumentation tests to verify that this is properly done. BUG=711770 ========== to ========== Last_n: instrumentation tests to verify timely snapshot deletion. Adds new tests to RecentTabsTest.java to verify that offline snapshots created by last_n are properly deleted when successfully navigating to another page and upon tab closure. These actions are important to comply with privacy requirements for this feature. This change re-adds instrumentation tests previously committed along with a related bugfix [1] that were reverted due to them being flaky. It seems that the underlying issue causing the flakyness has been fixed [2] [3] so these should now work as expected. [1] https://codereview.chromium.org/2824623002/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted) [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490 BUG=711770 ==========
Description was changed from ========== Last_n: instrumentation tests to verify timely snapshot deletion. Adds new tests to RecentTabsTest.java to verify that offline snapshots created by last_n are properly deleted when successfully navigating to another page and upon tab closure. These actions are important to comply with privacy requirements for this feature. This change re-adds instrumentation tests previously committed along with a related bugfix [1] that were reverted due to them being flaky. It seems that the underlying issue causing the flakyness has been fixed [2] [3] so these should now work as expected. [1] https://codereview.chromium.org/2824623002/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted) [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490 BUG=711770 ========== to ========== Last_n: instrumentation tests to verify timely snapshot deletion. Adds new tests to RecentTabsTest.java to verify that offline snapshots created by last_n are properly deleted when successfully navigating to another page and upon tab closure. These actions are important to comply with privacy requirements for this feature. This change re-adds instrumentation tests previously committed along with a related bugfix [1] that were reverted due to them being flaky. It seems that the underlying issue causing the flakyness has been fixed [2] [3] so these should now work as expected. [1] https://codereview.chromium.org/2824623002/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted) [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490 BUG=711770 ==========
The CQ bit was checked by carlosk@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...
carlosk@chromium.org changed reviewers: + dewittj@chromium.org, dtrainor@chromium.org
dewittj@: Please review changes in OfflinePageBridge.java dtrainor@: Please review changes in RecentTabsTest.java
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dewittj@chromium.org changed reviewers: - dtrainor@chromium.org
(dtrainor to CC) lgtm - just nits at this point. Didn't these tests get reverted due to flakiness a while ago? Are you certain they won't flake on the waterfall? https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:91: // Ensure we start in an offline state. this comment is no longer true. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:213: waitForPageWithClientId(firstTabClientId); nit: change the signature of this to return an OfflinePageItem. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:215: Assert.assertFalse(tab.equals(tabModelSelector.getCurrentTab())); This assert is in a confusing place. Why would tabs change around while waiting for a page? https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:303: @MainThread Why not just do the ui thread hop inside here? https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:316: public void assertDeleted() throws InterruptedException { I'd reword this to blockUntilDeleted
oh, just read the cl description in detail, nm about flake question.
Thanks. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:91: // Ensure we start in an offline state. On 2017/07/18 20:45:30, dewittj wrote: > this comment is no longer true. Done. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:213: waitForPageWithClientId(firstTabClientId); On 2017/07/18 20:45:30, dewittj wrote: > nit: change the signature of this to return an OfflinePageItem. Done. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:215: Assert.assertFalse(tab.equals(tabModelSelector.getCurrentTab())); On 2017/07/18 20:45:30, dewittj wrote: > This assert is in a confusing place. Why would tabs change around while waiting > for a page? This is mainly a confirmation that the newly created tab is now active, instead of the initial one. Knowing which tab is active is a very important part of these tests so I think it merits the checking. I also believe this makes the logic bit clearer to someone new to these tests. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:303: @MainThread On 2017/07/18 20:45:30, dewittj wrote: > Why not just do the ui thread hop inside here? If you look at the places this method is called from they also need to run other operations on the UI thread. I thought about having a helper method that would do everything that's expected when using this class but there is no use case for it yet (because of what I explained in the paragraph above). https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:316: public void assertDeleted() throws InterruptedException { On 2017/07/18 20:45:30, dewittj wrote: > I'd reword this to blockUntilDeleted Between the block and the assert, I think the latter makes it clearer that this will fail/assert if the item is not deleted. Block doesn't seem to convey that so well. WDYT?
still lgtm, but I do disagree on some of the finer points :) https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:215: Assert.assertFalse(tab.equals(tabModelSelector.getCurrentTab())); On 2017/07/18 21:42:18, carlosk wrote: > On 2017/07/18 20:45:30, dewittj wrote: > > This assert is in a confusing place. Why would tabs change around while > waiting > > for a page? > > This is mainly a confirmation that the newly created tab is now active, instead > of the initial one. Knowing which tab is active is a very important part of > these tests so I think it merits the checking. > > I also believe this makes the logic bit clearer to someone new to these tests. Then I would move it up to exactly when you are switching the tab (after line 212) https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:303: @MainThread On 2017/07/18 21:42:18, carlosk wrote: > On 2017/07/18 20:45:30, dewittj wrote: > > Why not just do the ui thread hop inside here? > > If you look at the places this method is called from they also need to run other > operations on the UI thread. > > I thought about having a helper method that would do everything that's expected > when using this class but there is no use case for it yet (because of what I > explained in the paragraph above). I agree that it is slightly more efficient for the bots to do all the things on the UI thread, but from a reading the test perspective, this setup is much more convoluted. If the class owns its own thread hopping then the test doesn't have to worry about it. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:316: public void assertDeleted() throws InterruptedException { On 2017/07/18 21:42:18, carlosk wrote: > On 2017/07/18 20:45:30, dewittj wrote: > > I'd reword this to blockUntilDeleted > > Between the block and the assert, I think the latter makes it clearer that this > will fail/assert if the item is not deleted. Block doesn't seem to convey that > so well. > WDYT? One way conveys that it's an assert better, and one way conveys that it blocks better. I'd say that for reading the test, knowledge that this blocks is more important (since all tests could timeout anyway).
Refactored deletion monitoring following your suggestion and it does look better. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java (right): https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:215: Assert.assertFalse(tab.equals(tabModelSelector.getCurrentTab())); On 2017/07/18 22:00:08, dewittj wrote: > On 2017/07/18 21:42:18, carlosk wrote: > > On 2017/07/18 20:45:30, dewittj wrote: > > > This assert is in a confusing place. Why would tabs change around while > > waiting > > > for a page? > > > > This is mainly a confirmation that the newly created tab is now active, > instead > > of the initial one. Knowing which tab is active is a very important part of > > these tests so I think it merits the checking. > > > > I also believe this makes the logic bit clearer to someone new to these tests. > > Then I would move it up to exactly when you are switching the tab (after line > 212) Done. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:303: @MainThread On 2017/07/18 22:00:09, dewittj wrote: > On 2017/07/18 21:42:18, carlosk wrote: > > On 2017/07/18 20:45:30, dewittj wrote: > > > Why not just do the ui thread hop inside here? > > > > If you look at the places this method is called from they also need to run > other > > operations on the UI thread. > > > > I thought about having a helper method that would do everything that's > expected > > when using this class but there is no use case for it yet (because of what I > > explained in the paragraph above). > > I agree that it is slightly more efficient for the bots to do all the things on > the UI thread, but from a reading the test perspective, this setup is much more > convoluted. If the class owns its own thread hopping then the test doesn't have > to worry about it. The approach you suggested during our offline conversation -- method returning a Semaphore -- was indeed simpler and I switched to that. https://codereview.chromium.org/2833653002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/RecentTabsTest.java:316: public void assertDeleted() throws InterruptedException { On 2017/07/18 22:00:08, dewittj wrote: > On 2017/07/18 21:42:18, carlosk wrote: > > On 2017/07/18 20:45:30, dewittj wrote: > > > I'd reword this to blockUntilDeleted > > > > Between the block and the assert, I think the latter makes it clearer that > this > > will fail/assert if the item is not deleted. Block doesn't seem to convey that > > so well. > > WDYT? > > One way conveys that it's an assert better, and one way conveys that it blocks > better. I'd say that for reading the test, knowledge that this blocks is more > important (since all tests could timeout anyway). Ditto.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2833653002/#ps120001 (title: "Simplified page deletion monitoring")
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": 120001, "attempt_start_ts": 1500426188847320,
"parent_rev": "3ac6740efc469abbf65953dd0fcb72469562ce7f", "commit_rev":
"f6fd25d5826f1a62010b08037eb50c396b05d7a2"}
Message was sent while issue was closed.
Description was changed from ========== Last_n: instrumentation tests to verify timely snapshot deletion. Adds new tests to RecentTabsTest.java to verify that offline snapshots created by last_n are properly deleted when successfully navigating to another page and upon tab closure. These actions are important to comply with privacy requirements for this feature. This change re-adds instrumentation tests previously committed along with a related bugfix [1] that were reverted due to them being flaky. It seems that the underlying issue causing the flakyness has been fixed [2] [3] so these should now work as expected. [1] https://codereview.chromium.org/2824623002/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted) [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490 BUG=711770 ========== to ========== Last_n: instrumentation tests to verify timely snapshot deletion. Adds new tests to RecentTabsTest.java to verify that offline snapshots created by last_n are properly deleted when successfully navigating to another page and upon tab closure. These actions are important to comply with privacy requirements for this feature. This change re-adds instrumentation tests previously committed along with a related bugfix [1] that were reverted due to them being flaky. It seems that the underlying issue causing the flakyness has been fixed [2] [3] so these should now work as expected. [1] https://codereview.chromium.org/2824623002/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=592961 (restricted) [3] https://bugs.chromium.org/p/chromium/issues/detail?id=738490 BUG=711770 Review-Url: https://codereview.chromium.org/2833653002 Cr-Commit-Position: refs/heads/master@{#487727} Committed: https://chromium.googlesource.com/chromium/src/+/f6fd25d5826f1a62010b08037eb5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f6fd25d5826f1a62010b08037eb5...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2983953002/ by jdoerrie@chromium.org. The reason for reverting is: Likely cause of https://crbug.com/746699. testLastNPageIsDeletedUponNavigation is flaky.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
