Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689213008:
Make Safe Browsing Subresource Filter tests truly end-to-end. (Closed)
DescriptionMake Safe Browsing Subresource Filter tests truly end-to-end.
As part of deprecating ContentSubresourceFilterDriver, remove a usage in
SafeBrowsingServiceTest, where a mock version of the driver is used to check
whether the subresource filtering activation IPC message is sent after the user
clicks through an interstitial.
Instead, use this opportunity to make this a truly end-to-end test, which
checks activation by probing whether an artificially disallowed resource indeed
fails to load. Also extend the test with verification that filtering is again
activated when the page is navigated to a second time when the interstitial is
not shown again.
BUG=637415
Review-Url: https://codereview.chromium.org/2689213008
Cr-Commit-Position: refs/heads/master@{#451350}
Committed: https://chromium.googlesource.com/chromium/src/+/7031169283c2d08e5b88f20ad1a64ef09aaf4df4
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove leftover include. #
Dependent Patchsets: Messages
Total messages: 29 (17 generated)
Description was changed from ========== Make Safe Browsing Subresource Filter tests truly end-to-end. As part of deprecating ContentSubresourceFilterDriver, remove a usage in SafeBrowsingServiceTest, where a mock version of the driver is used to check whether the subresource filtering activation IPC message is sent after the user clicks through an interstitial. Instead, use this opportunity to make this a truly end-to-end test, which checks activation by probing whether an artificially disallowed resource indeed fails to load. Also extend the test with verification that filtering is again activated when the page is navigated to a second time when the interstitial is not shown again. BUG=637415 ========== to ========== Make Safe Browsing Subresource Filter tests truly end-to-end. As part of deprecating ContentSubresourceFilterDriver, remove a usage in SafeBrowsingServiceTest, where a mock version of the driver is used to check whether the subresource filtering activation IPC message is sent after the user clicks through an interstitial. Instead, use this opportunity to make this a truly end-to-end test, which checks activation by probing whether an artificially disallowed resource indeed fails to load. Also extend the test with verification that filtering is again activated when the page is navigated to a second time when the interstitial is not shown again. BUG=637415 ==========
engedy@chromium.org changed reviewers: + csharrison@chromium.org, nparker@chromium.org
@Nathan, @Charlie, please take a look. Context: I am deprecating ContentSubresourceFilterDriver, and I thought I might use this as an opportunity to simplify these browser tests and make them truly end-to-end. While my original intention was to remove testing of subresource filtering from this file entirely -- and indeed, most of the detailed browser tests are already done separately -- I would like to keep these two tests here so that we can have the entire flow tested with bells and whistles (and without duplicating the overhead of using real interstitials and a more realistic version of the SB database).
The CQ bit was checked by engedy@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...
https://codereview.chromium.org/2689213008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2689213008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:964: // subresource filtering should still be activated. The fact that subresource filtering should still be activated is not tested here, I wonder if you could include both "included_script" and "allowed_script" on a single page?
https://codereview.chromium.org/2689213008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2689213008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:964: // subresource filtering should still be activated. On 2017/02/15 18:51:21, Charlie Harrison wrote: > The fact that subresource filtering should still be activated is not tested > here, I wonder if you could include both "included_script" and "allowed_script" > on a single page? So it is tested to some degree by line 969, but I agree that having both an allowed and a disallowed resource would be the ideal solution. Given that the subresource_filter_browsertest.cc suffers from this problem too, I wanted to fix this overall in a separate CL. (Feel free to do so if you are interested.)
Oops you are right. This CL LGTM as-is.
The CQ bit was checked by engedy@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...
engedy@chromium.org changed reviewers: + estark@chromium.org - nparker@chromium.org
engedy@chromium.org changed reviewers: + mattm@chromium.org - estark@chromium.org
Load balancing reviews. @Matt, would you have time to review this from the Safe Browsing side?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nparker@chromium.org changed reviewers: + jialiul@chromium.org, nparker@chromium.org - mattm@chromium.org
Swapping in jiaiul for mattm, since Matt is probably not interested in these any more (correct me if I'm wrong Matt!)
On 2017/02/16 02:51:04, Nathan Parker wrote: > Swapping in jiaiul for mattm, since Matt is probably not interested in these any > more (correct me if I'm wrong Matt!) Thanks!
lgtm Thanks!
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2689213008/#ps20001 (title: "Remove leftover include.")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 engedy@chromium.org
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": 20001, "attempt_start_ts": 1487355538084080,
"parent_rev": "b0bf86f90366be4a4fbb9a37da1e1d23d45fee27", "commit_rev":
"7031169283c2d08e5b88f20ad1a64ef09aaf4df4"}
Message was sent while issue was closed.
Description was changed from ========== Make Safe Browsing Subresource Filter tests truly end-to-end. As part of deprecating ContentSubresourceFilterDriver, remove a usage in SafeBrowsingServiceTest, where a mock version of the driver is used to check whether the subresource filtering activation IPC message is sent after the user clicks through an interstitial. Instead, use this opportunity to make this a truly end-to-end test, which checks activation by probing whether an artificially disallowed resource indeed fails to load. Also extend the test with verification that filtering is again activated when the page is navigated to a second time when the interstitial is not shown again. BUG=637415 ========== to ========== Make Safe Browsing Subresource Filter tests truly end-to-end. As part of deprecating ContentSubresourceFilterDriver, remove a usage in SafeBrowsingServiceTest, where a mock version of the driver is used to check whether the subresource filtering activation IPC message is sent after the user clicks through an interstitial. Instead, use this opportunity to make this a truly end-to-end test, which checks activation by probing whether an artificially disallowed resource indeed fails to load. Also extend the test with verification that filtering is again activated when the page is navigated to a second time when the interstitial is not shown again. BUG=637415 Review-Url: https://codereview.chromium.org/2689213008 Cr-Commit-Position: refs/heads/master@{#451350} Committed: https://chromium.googlesource.com/chromium/src/+/7031169283c2d08e5b88f20ad1a6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7031169283c2d08e5b88f20ad1a6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
