SafeBrowising: Read and write V4Store from/to disk

components/safe_browsing_db/v4_store_unittest.cc

Index: components/safe_browsing_db/v4_store_unittest.cc
diff --git a/components/safe_browsing_db/v4_store_unittest.cc b/components/safe_browsing_db/v4_store_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..57e9c2b8cec8fb6e1f15e8ad1e1d51766b341a63
--- /dev/null
+++ b/components/safe_browsing_db/v4_store_unittest.cc
@@ -0,0 +1,138 @@
+// 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/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/time/time.h"
+#include "components/safe_browsing_db/v4_store.h"
+#include "components/safe_browsing_db/v4_store.pb.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/platform_test.h"
+
+namespace safe_browsing {
+
+class V4StoreTest : public PlatformTest {
+ public:
+  V4StoreTest() : task_runner_(new base::TestSimpleTaskRunner) {}
+
+  void SetUp() override {
+    PlatformTest::SetUp();
+
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    store_path_ = temp_dir_.path().AppendASCII("V4StoreTest.store");
+    DVLOG(1) << "store_path_: " << store_path_.value();
+  }
+
+  void TearDown() override {
+    base::DeleteFile(store_path_, false);
+    PlatformTest::TearDown();
+  }
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

Will task_runner_ destruction pump events?  If so, might be reasonable to have
task_runner_ after temp_dir_ in case outstanding tasks refer to tempfiles.

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Thanks! Didn't think about that.
The detail with which you do code reviews is mind-boggling. Thanks a lot!

+  base::ScopedTempDir temp_dir_;
+  base::FilePath store_path_;
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_F(V4StoreTest, TestReadFromEmptyFile) {
+  { base::ScopedFILE new_file(base::OpenFile(store_path_, "wb+")); }

[Scott Hess - ex-Googler, 2016/06/22 22:58:40]

I see what you're doing there, but I think the single-line scope isn't Chromium
acceptable.

How about just:
  base::CloseFile(base::OpenFile(...));
Maybe with a comment saying "Create an empty file"?

Or, if you want to be tricksy:
  base::ScopedFILE(base::OpenFile(...));
But don't be tricksy!

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

This formatting was done by "git cl format". Doesn't that make it acceptable? :)

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Really?  I think that's the first time I've ever seen a single-line non-empty
block.

Still, I think CloseFile(OpenFile(...)) may be more obvious.  I'll leave that to
your discretion.

[vakh (use Gerrit instead), 2016/06/24 18:03:50]

Done.

+
+  EXPECT_EQ(FILE_EMPTY_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestReadFromAbsentFile) {
+  EXPECT_EQ(FILE_UNREADABLE_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

I like busting out all the cases for testing!

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Acknowledged.

+
+TEST_F(V4StoreTest, TestReadFromInvalidContentsFile) {
+  std::string invalid_contents = "Chromium";
+  base::WriteFile(store_path_, invalid_contents.c_str(),
+                  invalid_contents.size());

[Scott Hess - ex-Googler, 2016/06/22 22:58:40]

const kInvalidContents[] = "Chromium";
base::WriteFile(store_path_, kInvalidContents, ARRAY_SIZE(kInvalidContents));

Or strlen().

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Done. Fits on a line too!

+
+  EXPECT_EQ(PROTO_PARSING_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestReadFromUnexpectedMagicFile) {
+  V4StoreFileFormat file_format;
+  file_format.set_magic_number(111);
+  std::string file_format_string;
+  file_format.SerializeToString(&file_format_string);
+  base::WriteFile(store_path_, file_format_string.c_str(),
+                  file_format_string.size());

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

data() rather than c_str().

Right now I'm wishing for a WriteFile() which takes a StringPiece, to be honest.

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

It's the same. data and c_str

+  EXPECT_EQ(UNEXPECTED_MAGIC_NUMBER_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestReadFromLowVersionFile) {
+  V4StoreFileFormat file_format;
+  file_format.set_magic_number(0x600D71FE);
+  file_format.set_version_number(2);
+  std::string file_format_string;
+  file_format.SerializeToString(&file_format_string);
+  base::WriteFile(store_path_, file_format_string.c_str(),
+                  file_format_string.size());

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

data().

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

See above.

+  EXPECT_EQ(FILE_VERSION_INCOMPATIBLE_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestReadFromNoHashPrefixInfoFile) {
+  V4StoreFileFormat file_format;
+  file_format.set_magic_number(0x600D71FE);
+  file_format.set_version_number(9);
+  std::string file_format_string;
+  file_format.SerializeToString(&file_format_string);
+  base::WriteFile(store_path_, file_format_string.c_str(),
+                  file_format_string.size());

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

data().  Or an anonymous WriteStringToFile() helper.  A WriteProtoToFile()
helper would help even more.

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Done.

+  EXPECT_EQ(HASH_PREFIX_INFO_MISSING_FAILURE,
+            V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestReadFromNoHashPrefixesFile) {
+  V4StoreFileFormat file_format;
+  file_format.set_magic_number(0x600D71FE);
+  file_format.set_version_number(9);
+  ListUpdateResponse* list_update_response =
+      file_format.mutable_list_update_response();
+  list_update_response->set_platform_type(LINUX_PLATFORM);
+
+  std::string file_format_string;
+  file_format.SerializeToString(&file_format_string);
+  base::WriteFile(store_path_, file_format_string.c_str(),
+                  file_format_string.size());
+  EXPECT_EQ(READ_SUCCESS, V4Store(task_runner_, store_path_).ReadFromDisk());
+}
+
+TEST_F(V4StoreTest, TestWriteNoResponseType) {
+  ListUpdateResponse list_update_response;
+  EXPECT_EQ(
+      INVALID_RESPONSE_TYPE_FAILURE,
+      V4Store(task_runner_, store_path_).WriteToDisk(list_update_response));
+}

[Scott Hess - ex-Googler, 2016/06/22 22:58:40]

Hmm.  Now my DCHECK suggestions are looking wrong.  I guess if you're going to
test the error cases, you have to be able to complete the test.

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

Acknowledged.

+
+TEST_F(V4StoreTest, TestWritePartialResponseType) {
+  ListUpdateResponse list_update_response;
+  list_update_response.set_response_type(ListUpdateResponse::PARTIAL_UPDATE);
+  EXPECT_EQ(
+      INVALID_RESPONSE_TYPE_FAILURE,
+      V4Store(task_runner_, store_path_).WriteToDisk(list_update_response));
+}
+
+TEST_F(V4StoreTest, TestWriteFullResponseType) {
+  ListUpdateResponse list_update_response;
+  list_update_response.set_response_type(ListUpdateResponse::FULL_UPDATE);
+  list_update_response.set_new_client_state("test_client_state");
+  std::unique_ptr<V4Store> write_store(new V4Store(task_runner_, store_path_));

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

Could this just be a V4Store without the unique_ptr?

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Done.

+  EXPECT_EQ(WRITE_SUCCESS, write_store->WriteToDisk(list_update_response));
+
+  std::unique_ptr<V4Store> read_store(new V4Store(task_runner_, store_path_));

[Scott Hess - ex-Googler, 2016/06/22 22:58:40]

Also here?

[vakh (use Gerrit instead), 2016/06/23 23:36:21]

Done.

+  EXPECT_EQ(READ_SUCCESS, read_store->ReadFromDisk());
+  EXPECT_EQ("test_client_state", read_store->state_);
+}
+
+}  // namespace safe_browsing