Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820933002:
[subresource_filter] add //chrome level unit test harness (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] add //chrome level unit test harness
The subresource_filter code is getting more dependent on //chrome level
things like content settings. Right now it is tricky to get end to end
testing without bringing up the whole browser, even for very simple
tests.
This patch adds a test harness which sets up all the necessary
dependencies for subresource filtering. This should make testing new
features easy and fast.
BUG=709768
Review-Url: https://codereview.chromium.org/2820933002
Cr-Commit-Position: refs/heads/master@{#466270}
Committed: https://chromium.googlesource.com/chromium/src/+/ac0e81d240f79c8fe074ba135e28bf34e6ed6ed8
Patch Set 1 #Patch Set 2 : add some GN deps #Patch Set 3 : add infobar serivice tab helper (fix android) #Patch Set 4 : add a few #includes (trybots prev) #
Total comments: 24
Patch Set 5 : engedy review #Patch Set 6 : fix leaks #Patch Set 7 : remove dep #Messages
Total messages: 46 (31 generated)
The CQ bit was checked by csharrison@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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 csharrison@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...)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + engedy@chromium.org
engedy WDYT about this patch? I felt like we've been relatively constrained in //chrome land, forcing us to add browser tests for simple tests. This test harness is admittedly a little intense though. LMK what you think.
I will never say `no` to more tests, so I think this is awesome, thanks for putting it together! LGTM % comments! https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:73: bool shown_for_navigation() { return shown_for_navigation_; } nit: How about {did_show|shown}_ui_for_navigation()? I think Tanja might be renaming the underlying member variable in a concurrent CL. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:8: #include "base/feature_list.h" Nice, I didn't find a single missing or superfluous include. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:67: auto* safe_browsing_service = This seems a bit hacky. :) Is there any nicer solution here? https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); nit: ChromeUnitTestSuiteInitializer::OnTestEnd destroys the TestingBrowserProcess, so this might not be needed. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:155: base::test::ScopedFeatureList scoped_feature_list_; Let's move this up by one, so that they get destroyed in reverse order of initialization. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/BUILD.gn (right): https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/BUILD.gn:57: "//components/safe_browsing_db:test_database_manager", I'm not sure how strongly this is enforced, but someone complained on another CL that the subresource filter's headers include a lot of other headers from other components, but don't expose the source component as a public deps. This might be problematic at least in theory because the dependent translation unit that will indirectly include those headers will not inherit include directory configs. Consider moving this to public_deps (probably many other deps need to be moved, but let's do that in another CL). https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h (right): https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:9: #include <set> nit: No need to include this in the header, it is not using this to any greater degree than the base class. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:12: #include "components/safe_browsing_db/hit_report.h" nit: Unused? https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:14: #include "content/public/common/resource_type.h" nit: Unused? https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:15: #include "url/gurl.h" nit: Fwd-declaration below seems sufficient.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Tried to address most comments https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:73: bool shown_for_navigation() { return shown_for_navigation_; } On 2017/04/20 12:34:33, engedy wrote: > nit: How about {did_show|shown}_ui_for_navigation()? I think Tanja might be > renaming the underlying member variable in a concurrent CL. Done, I've also changed the underlying member to be consistent. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:8: #include "base/feature_list.h" On 2017/04/20 12:34:33, engedy wrote: > Nice, I didn't find a single missing or superfluous include. If only this were automated :) https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:67: auto* safe_browsing_service = On 2017/04/20 12:34:33, engedy wrote: > This seems a bit hacky. :) Is there any nicer solution here? This is cribbed from notification_image_reporter_unittest.cc. I tried to copy the instructions at the top of test_safe_browsing_service.h, but I don't think they are robust to unit test harnesses. I've added a TODO to clean this up in the meantime. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 12:34:33, engedy wrote: > nit: ChromeUnitTestSuiteInitializer::OnTestEnd destroys the > TestingBrowserProcess, so this might not be needed. The Shutdown() and run loop is needed, to ensure the service tears down correctly. I don't think the Set* methods are needed though so I've removed them. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:155: base::test::ScopedFeatureList scoped_feature_list_; On 2017/04/20 12:34:33, engedy wrote: > Let's move this up by one, so that they get destroyed in reverse order of > initialization. Done. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/BUILD.gn (right): https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/BUILD.gn:57: "//components/safe_browsing_db:test_database_manager", On 2017/04/20 12:34:33, engedy wrote: > I'm not sure how strongly this is enforced, but someone complained on another CL > that the subresource filter's headers include a lot of other headers from other > components, but don't expose the source component as a public deps. This might > be problematic at least in theory because the dependent translation unit that > will indirectly include those headers will not inherit include directory > configs. > > Consider moving this to public_deps (probably many other deps need to be moved, > but let's do that in another CL). Done. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h (right): https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:9: #include <set> On 2017/04/20 12:34:33, engedy wrote: > nit: No need to include this in the header, it is not using this to any greater > degree than the base class. Done. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:12: #include "components/safe_browsing_db/hit_report.h" On 2017/04/20 12:34:33, engedy wrote: > nit: Unused? Used for safe_browsing::ThreatSource, but it isn't necessary. Done. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:14: #include "content/public/common/resource_type.h" On 2017/04/20 12:34:33, engedy wrote: > nit: Unused? Ditto, done. https://codereview.chromium.org/2820933002/diff/60001/components/subresource_... components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h:15: #include "url/gurl.h" On 2017/04/20 12:34:33, engedy wrote: > nit: Fwd-declaration below seems sufficient. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:67: auto* safe_browsing_service = On 2017/04/20 15:55:20, Charlie Harrison wrote: > On 2017/04/20 12:34:33, engedy wrote: > > This seems a bit hacky. :) Is there any nicer solution here? > > This is cribbed from notification_image_reporter_unittest.cc. > > I tried to copy the instructions at the top of test_safe_browsing_service.h, but > I don't think they are robust to unit test harnesses. > > I've added a TODO to clean this up in the meantime. Acknowledged. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 15:55:20, Charlie Harrison wrote: > On 2017/04/20 12:34:33, engedy wrote: > > nit: ChromeUnitTestSuiteInitializer::OnTestEnd destroys the > > TestingBrowserProcess, so this might not be needed. > > The Shutdown() and run loop is needed, to ensure the service tears down > correctly. I don't think the Set* methods are needed though so I've removed > them. Acknowledged.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
Thanks Balazs, this should make the smart UI changes much more testable.
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 csharrison@chromium.org
Let me rebase this on the NavigationSimulator fixup CL, actually
csharrison@chromium.org changed reviewers: + clamy@chromium.org
NVM it isn't necessary. +clamy would you PTAL at this trivial nav simulator CL?
Thanks! Lgtm.
The CQ bit was checked by csharrison@chromium.org
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 csharrison@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/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 16:36:53, engedy wrote: > On 2017/04/20 15:55:20, Charlie Harrison wrote: > > On 2017/04/20 12:34:33, engedy wrote: > > > nit: ChromeUnitTestSuiteInitializer::OnTestEnd destroys the > > > TestingBrowserProcess, so this might not be needed. > > > > The Shutdown() and run loop is needed, to ensure the service tears down > > correctly. I don't think the Set* methods are needed though so I've removed > > them. > > Acknowledged. Actually, they are needed. We need to reset the objects and pump the run loop to ensure everything cleans up (both the verified ruleset handles and the SB service). I'm re-adding this logic.
https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 19:32:48, Charlie Harrison wrote: > On 2017/04/20 16:36:53, engedy wrote: > > On 2017/04/20 15:55:20, Charlie Harrison wrote: > > > On 2017/04/20 12:34:33, engedy wrote: > > > > nit: ChromeUnitTestSuiteInitializer::OnTestEnd destroys the > > > > TestingBrowserProcess, so this might not be needed. > > > > > > The Shutdown() and run loop is needed, to ensure the service tears down > > > correctly. I don't think the Set* methods are needed though so I've removed > > > them. > > > > Acknowledged. > > Actually, they are needed. We need to reset the objects and pump the run loop to > ensure everything cleans up (both the verified ruleset handles and the SB > service). I'm re-adding this logic. Ack, sorry for the noise.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2820933002/#ps100001 (title: "fix leaks")
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2820933002/#ps100001 (title: "fix leaks")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2826143002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2820933002/#ps120001 (title: "remove dep")
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": 1492748978889960,
"parent_rev": "8edd4a3b7543a5d2038bad4fabf58ab9865c1e8c", "commit_rev":
"7955ec87d05cae293850c29906c37afecf1612cc"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492748978889960,
"parent_rev": "019b1356d38d23a10d02b636eaad8f1200fd804e", "commit_rev":
"ac0e81d240f79c8fe074ba135e28bf34e6ed6ed8"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] add //chrome level unit test harness The subresource_filter code is getting more dependent on //chrome level things like content settings. Right now it is tricky to get end to end testing without bringing up the whole browser, even for very simple tests. This patch adds a test harness which sets up all the necessary dependencies for subresource filtering. This should make testing new features easy and fast. BUG=709768 ========== to ========== [subresource_filter] add //chrome level unit test harness The subresource_filter code is getting more dependent on //chrome level things like content settings. Right now it is tricky to get end to end testing without bringing up the whole browser, even for very simple tests. This patch adds a test harness which sets up all the necessary dependencies for subresource filtering. This should make testing new features easy and fast. BUG=709768 Review-Url: https://codereview.chromium.org/2820933002 Cr-Commit-Position: refs/heads/master@{#466270} Committed: https://chromium.googlesource.com/chromium/src/+/ac0e81d240f79c8fe074ba135e28... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ac0e81d240f79c8fe074ba135e28... |
