V4LDM: Add check for HashPrefix match and some tests

components/safe_browsing_db/v4_local_database_manager_unittest.cc

Index: components/safe_browsing_db/v4_local_database_manager_unittest.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager_unittest.cc b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6c6d3cc77017adb4fdc72705c0f341a180d31c22
--- /dev/null
+++ b/components/safe_browsing_db/v4_local_database_manager_unittest.cc
@@ -0,0 +1,137 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "base/test/test_simple_task_runner.h"
+#include "components/safe_browsing_db/v4_database.h"
+#include "components/safe_browsing_db/v4_local_database_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/platform_test.h"
+
+namespace safe_browsing {
+
+struct FakeV4DatabaseConfig {
+  MatchedHashPrefixMap matched_hash_prefix_map_;

[Nathan Parker, 2016/08/15 19:06:41]

If this is the only member, you could just pass that w/o the wrapper class. 
Less code is better.

[vakh (use Gerrit instead), 2016/08/17 18:23:06]

I think we might need more fields later, but since I am not adding right now, I
have removed the wrapper class.

+};
+
+class FakeV4Database : public V4Database {
+ public:
+  FakeV4Database(const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
+                 std::unique_ptr<StoreMap> store_map,
+                 const FakeV4DatabaseConfig& config)
+      : V4Database(db_task_runner, std::move(store_map)), config_(config) {}
+
+  void GetStoresMatchingFullHash(
+      const FullHash& full_hash,
+      const base::hash_set<UpdateListIdentifier>& stores_to_look,
+      MatchedHashPrefixMap* matched_hash_prefix_map) override {
+    for (const auto it : config_.matched_hash_prefix_map_) {

[Nathan Parker, 2016/08/15 19:06:41]

I think the whole loop could be:

matched_hash_prefix_map->erase();
*matched_hash_prefix_map = config_.matched_hash_prefix_map_;

[Scott Hess - ex-Googler, 2016/08/15 20:57:40]

Wouldn't the erase() be redundant?

[vakh (use Gerrit instead), 2016/08/17 18:23:06]

Done.

+      (*matched_hash_prefix_map)[it.first] = it.second;
+    }
+  }
+
+ private:
+  const FakeV4DatabaseConfig& config_;
+};
+
+class V4LocalDatabaseManagerTest : public PlatformTest {
+ public:
+  V4LocalDatabaseManagerTest() : task_runner_(new base::TestSimpleTaskRunner) {}
+
+  void SetUp() override {
+    PlatformTest::SetUp();
+
+    ASSERT_TRUE(base_dir_.CreateUniqueTempDir());
+    DVLOG(1) << "base_dir_: " << base_dir_.path().value();
+
+    v4_local_database_manager_ =
+        make_scoped_refptr(new V4LocalDatabaseManager(base_dir_.path()));
+    v4_local_database_manager_->SetTaskRunnerForTest(task_runner_);
+
+    SetupLocalDatabaseManager();
+  }
+
+  void TearDown() override {
+    v4_local_database_manager_->StopOnIOThread(true);
+
+    // Force destruction of the database.
+    task_runner_->RunPendingTasks();
+    base::RunLoop().RunUntilIdle();
+
+    PlatformTest::TearDown();
+  }
+
+  void SetupLocalDatabaseManager() {
+    v4_local_database_manager_->StartOnIOThread(NULL, V4ProtocolConfig());
+
+    task_runner_->RunPendingTasks();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void ReplaceV4Database(const FakeV4DatabaseConfig& config) {
+    v4_local_database_manager_->v4_database_.reset(
+        new FakeV4Database(task_runner_, base::MakeUnique<StoreMap>(), config));
+  }
+
+  void ForceDisableLocalDatabaseManager() {
+    v4_local_database_manager_->enabled_ = false;
+  }
+
+  base::ScopedTempDir base_dir_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  content::TestBrowserThreadBundle thread_bundle_;
+  scoped_refptr<V4LocalDatabaseManager> v4_local_database_manager_;
+};
+
+TEST_F(V4LocalDatabaseManagerTest, TestGetThreatSource) {
+  EXPECT_EQ(ThreatSource::LOCAL_PVER4,
+            v4_local_database_manager_->GetThreatSource());
+}
+
+TEST_F(V4LocalDatabaseManagerTest, TestIsSupported) {
+  EXPECT_TRUE(v4_local_database_manager_->IsSupported());
+}
+
+TEST_F(V4LocalDatabaseManagerTest, TestCanCheckUrl) {
+  EXPECT_TRUE(v4_local_database_manager_->CanCheckUrl(GURL("http://a/")));

[Scott Hess - ex-Googler, 2016/08/15 20:57:39]

Just wondering ... is there any possibility that an unqualified hostname like
this would lead to odd special cases?  It might be reasonable to have a legit
FQDN, like example.com or abc.xyz.

[vakh (use Gerrit instead), 2016/08/17 18:23:06]

Done.

+  EXPECT_TRUE(v4_local_database_manager_->CanCheckUrl(GURL("https://a/")));
+  EXPECT_TRUE(v4_local_database_manager_->CanCheckUrl(GURL("ftp://a/")));
+  EXPECT_FALSE(v4_local_database_manager_->CanCheckUrl(GURL("adp://a/")));
+}
+
+TEST_F(V4LocalDatabaseManagerTest,
+       TestCheckBrowseUrlWithEmptyStoresReturnsNoMatch) {
+  // Both the stores are empty right now so CheckBrowseUrl should return true.
+  EXPECT_TRUE(
+      v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr));
+}
+
+TEST_F(V4LocalDatabaseManagerTest, TestCheckBrowseUrlWithFakeDbReturnsMatch) {
+  FakeV4DatabaseConfig config;
+  config.matched_hash_prefix_map_[GetUrlMalwareId()] = HashPrefix("aaaa");
+  ReplaceV4Database(config);
+
+  // The fake database returns a matched hash prefix.
+  EXPECT_FALSE(
+      v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr));

[Scott Hess - ex-Googler, 2016/08/15 20:57:40]

Looking at this line ... it appears non-self-documenting.  Like maybe
"CheckBrowseURL" should return whether the act of checking was successful or
not, with a by-reference return for the results of the check.  But if it's a
check with built-in error handling, it should be something else with a yes/no
response, like "IsBrowseUrlSafe" or "IsBrowseUrlUnsafe".  Just IMHO, I might be
overthinking it.

Also, I'm not entirely clear how HashPrefix("aaaa") leads to this check
matching, since "aaaa" and "a" aren't obviously related.  For all I can tell, a
check against "http://b/" would return the same thing.

[vakh (use Gerrit instead), 2016/08/17 18:23:06]

I agree with that sentiment, but at the moment, I want to make the new DBManager
have the exact same API as the one it is replacing so that the clients don't
need to change.
Once the migration is complete, I'll consider changing the API name. I may not
get to it soon, but it is not urgent, IMO.
That's true. I am injecting a fake return value from a dependency (V4Database)
and testing that the code in the module under test (V4LDBM) does what's
expected. It does not matter if the fake value from the dependency module is the
exact value this code will encounter in production.

[Scott Hess - ex-Googler, 2016/08/17 23:04:38]

Do you mean that the fake will always hit?  OK, I guess, though it seems like
the kind of thing where later you're going to have to adjust all of these things
when the fake becomes more precise.

[vakh (use Gerrit instead), 2016/08/17 23:51:07]

Yes, that's the case this test is targeting.
Perhaps. But it gives me confidence that the code I am committing today works as
intended and no other changes break it until it is replaced with more precise
code.

+}
+
+TEST_F(V4LocalDatabaseManagerTest,
+       TestCheckBrowseUrlReturnsNoMatchWhenDisabled) {
+  FakeV4DatabaseConfig config;
+  config.matched_hash_prefix_map_[GetUrlMalwareId()] = HashPrefix("aaaa");
+  ReplaceV4Database(config);
+
+  // The same URL returns |false| in the previous test because
+  // v4_local_database_manager_ is enabled.
+  ForceDisableLocalDatabaseManager();
+
+  EXPECT_TRUE(
+      v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr));
+}
+
+}  // namespace safe_browsing