Make safe browsing browser tests exercise more code |
||||||
Issue descriptionIt would be nice to have test infrastructure which mock out Android APIs / V4 server to use in browsertests, to exercise metadata parsing code in e2e tests.
,
Oct 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df479a79836e03a72bfef55b54d4b2c378677bfe commit df479a79836e03a72bfef55b54d4b2c378677bfe Author: Charles Harrison <csharrison@chromium.org> Date: Tue Oct 17 18:55:06 2017 Introduce V4GetHashInterceptor for safe browsing end to end tests The class is a URLRequestInterceptor which intercepts URL requests to the safe browsing endpoint https://safebrowsing.googleapis.com/v4 and responds with canned responses. This allows browsertests to additionally test that V4 metadata parsing is working as intended. Bug: 775530 Change-Id: I40d4504768b46d00259f13f2900be98fa6a713c1 Reviewed-on: https://chromium-review.googlesource.com/719816 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Cr-Commit-Position: refs/heads/master@{#509467} [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/BUILD.gn [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc [add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/v4_get_hash_interceptor_browsertest.cc [delete] https://crrev.com/7748cea2ab5767bef12997c2a5225b566df02d96/chrome/browser/safe_browsing/v4_test_utils.cc [delete] https://crrev.com/7748cea2ab5767bef12997c2a5225b566df02d96/chrome/browser/safe_browsing/v4_test_utils.h [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/subresource_filter/subresource_filter_browsertest.cc [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/test/BUILD.gn [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/BUILD.gn [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_database.h [add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_get_hash_interceptor.cc [add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_get_hash_interceptor.h [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_protocol_manager_util.cc [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_protocol_manager_util.h [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_test_util.cc [modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_test_util.h
,
Oct 17 2017
Welp, just talked to mmenke@ who mentioned URLRequestInterceptor is deprecated and will break soon with the network service. I'll fix this by converting it into an embedded test server callback, with some associated host resolver mappings.
,
Oct 20 2017
There is work on the Safe Browsing backend team to produce a test server that can walk through a bunch of update scenarios for a client like Chrome. Would that be the kind of e2e test you're thinking of?
,
Oct 20 2017
Ah, I see now you said browser tests, so never mind my last comment.
,
Oct 20 2017
nparker: good to know! Yeah I've updated the summary to be a bit more clear. The main reason I started working on this was that I had a CL which added metadata parsing but it was only exercised in unit tests. As I am not very familiar with the safe browsing codebase, this made me a bit uncomfortable and I wanted to make sure I didn't introduce bugs.
,
Oct 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ad8ea07f746dc869f51dd2573599c236a9b5932 commit 3ad8ea07f746dc869f51dd2573599c236a9b5932 Author: Charles Harrison <csharrison@chromium.org> Date: Wed Oct 25 03:52:32 2017 Replace the V4 URLRequestInterceptor with a embedded test server handler URLRequestInterceptors are deprecated and will not work in the network service. This CL does a few things: 1. Introduce a global g_sbv4_url_prefix_for_testing, which is used in production code if it is set. This is non-ideal, but it turned out to be quite difficult to redirect the request using only host resolver rules. 2. Replace the V4 URLRequestInterceptor with a EmbeddedTestServer request handler, which is compatible with the network service. 3. Further improve the V4 handler by a. Only intercepting requests to /v4/fullHashes:find. b. Fully support requests with only hash prefixes, i.e. if a request comes through that matches multiple URLs, respond with multiple ThreatMatches. 4. Reduces the scope of some of the browser tests because the test server should have the handlers registered before being started. Bug: 775530 Change-Id: I0eba45b72eece5791224e54082a7b442e9b7f6d0 Reviewed-on: https://chromium-review.googlesource.com/726259 Reviewed-by: Varun Khaneja <vakh@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#511357} [rename] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/chrome/browser/safe_browsing/v4_embedded_test_server_browsertest.cc [modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/chrome/test/BUILD.gn [modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/BUILD.gn [modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_database.h [add] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_embedded_test_server_util.cc [add] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_embedded_test_server_util.h [delete] https://crrev.com/c8ca971ca4303f3d5075896d072346b7ba656b97/components/safe_browsing/db/v4_get_hash_interceptor.cc [delete] https://crrev.com/c8ca971ca4303f3d5075896d072346b7ba656b97/components/safe_browsing/db/v4_get_hash_interceptor.h [modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_protocol_manager_util.cc [modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_protocol_manager_util.h
,
Oct 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d commit a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d Author: Charles Harrison <csharrison@chromium.org> Date: Mon Oct 30 19:07:44 2017 [subresource_filter] Add a full hash request intercepting browsertest This patch does a few things: 1. Beef up the TestSafeBrowsingDatabaseHelper, so callers can optionally, pass in a custom TestV4GetHashProtocolManagerFactory. If this is null, then the helper will not mock the v4 hash protocol manager. This causes real API requests to go out to the network. 2. Make TestV4Store::MarkPrefixAsBad support multiple prefixes per prefix size. 3. Minor cleanups of names and std::make_unique 4. Add a new browser test suite for subresource_filter which uses the v4 embedded_test_server request handler, combined with the new TestSafeBrowsingDatabaseHelper, to mock out v4 API responses for the subresource filter list. This tests enforcement and warning variants. Bug: 775530 , 737201 Change-Id: Id26b676194fd33c28737bcb693e47ca75145f82f Reviewed-on: https://chromium-review.googlesource.com/730405 Reviewed-by: Shivani Sharma <shivanisha@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#512567} [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/test_safe_browsing_database_helper.h [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/v4_embedded_test_server_browsertest.cc [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_browsertest.cc [add] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_intercepting_browsertest.cc [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_browsertest.cc [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/test/BUILD.gn [modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/components/safe_browsing/db/v4_test_util.cc
,
Nov 10 2017
,
Jan 9 2018
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18708f498d97f6c157d82ad0e6f558252791a733 commit 18708f498d97f6c157d82ad0e6f558252791a733 Author: Charles Harrison <csharrison@chromium.org> Date: Tue Jan 09 19:27:14 2018 Safe Browsing e2e instrumentation tests on Android This CL adds a MockSafeBrowsingApiHandler which vends fake response JSON. It is used by a simple test harness which checks that an interstitial is properly shown. Bug: 775530 Change-Id: I5a8c21edd57a6a57c1de85feed1205502799673b Reviewed-on: https://chromium-review.googlesource.com/850339 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#528066} [modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/BUILD.gn [modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/java_sources.gni [modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/DEPS [add] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java [add] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java
,
Jan 9 2018
I'm going to close this issue out, despite the fact that it's always nicer to make tests exercise *even more* code :) To sum up the work: 1. On Android: MockSafeBrowsingApiHandler can be used to mock out SafetyNet JSON responses. This can only be used in Java tests but should be relatively easy to use once we get native browser_tests working on Android. 2. On desktop: We provide a fake safe browsing v4 server which can respond with arbitrary full hash responses, via a custom embedded test server handler. We don't currently support update requests though. This should be used in conjunction with the test v4 db for hash prefixes. For new code that sits below ThreatMetadata, or for instrumentation tests on Android, consider using these new tools!
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd commit 7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd Author: Joy Ming <jming@chromium.org> Date: Tue Jan 09 22:01:49 2018 Revert "Safe Browsing e2e instrumentation tests on Android" This reverts commit 18708f498d97f6c157d82ad0e6f558252791a733. Reason for revert: The test failed on KitKat Phone Tester (dbg), See crbug.com/800514 Original change's description: > Safe Browsing e2e instrumentation tests on Android > > This CL adds a MockSafeBrowsingApiHandler which vends fake response > JSON. It is used by a simple test harness which checks that an > interstitial is properly shown. > > Bug: 775530 > Change-Id: I5a8c21edd57a6a57c1de85feed1205502799673b > Reviewed-on: https://chromium-review.googlesource.com/850339 > Commit-Queue: Charlie Harrison <csharrison@chromium.org> > Reviewed-by: Varun Khaneja <vakh@chromium.org> > Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528066} TBR=tedchoc@chromium.org,csharrison@chromium.org,vakh@chromium.org Change-Id: I3c5ee49892684a2a12718326b55bc32da58a7ab1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 775530 Reviewed-on: https://chromium-review.googlesource.com/857924 Reviewed-by: Joy Ming <jming@chromium.org> Commit-Queue: Joy Ming <jming@chromium.org> Cr-Commit-Position: refs/heads/master@{#528132} [modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/BUILD.gn [modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/java_sources.gni [modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/javatests/DEPS [delete] https://crrev.com/0acaa0b094e76002120a3e6f3e9492f2e9c64b0c/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java [delete] https://crrev.com/0acaa0b094e76002120a3e6f3e9492f2e9c64b0c/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java
,
Jan 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a675185d4ed064433611c309b85b9aad971bf0f2 commit a675185d4ed064433611c309b85b9aad971bf0f2 Author: Charles Harrison <csharrison@chromium.org> Date: Fri Jan 12 21:02:52 2018 Reland: Safe Browsing e2e instrumentation tests on Android This CL adds a MockSafeBrowsingApiHandler which vends fake response JSON. It is used by a simple test harness which checks that an interstitial is properly shown. Previously landed here: https://chromium-review.googlesource.com/c/chromium/src/+/850339 TBR=vakh@chromium.org Bug: 775530 ,800514 Change-Id: I4834caef5323a2450b21b54e9307d316b44d8b5b Reviewed-on: https://chromium-review.googlesource.com/860622 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#529062} [modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/BUILD.gn [modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/java_sources.gni [modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/DEPS [add] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java [add] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by csharrison@chromium.org
, Oct 17 2017