[IndexedDB] Mojo testing harness.

content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc

Index: content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc
diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc b/content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6be1ec7d1fb151a23895baa84a83675a223d1216
--- /dev/null
+++ b/content/browser/indexed_db/indexed_db_dispatcher_host_unittest.cc
@@ -0,0 +1,306 @@
+// Copyright 2017 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 "content/browser/indexed_db/indexed_db_dispatcher_host.h"
+
+#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/single_thread_task_runner.h"
+#include "base/strings/utf_offset_string_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "content/browser/browser_thread_impl.h"
+#include "content/browser/indexed_db/indexed_db_callbacks.h"
+#include "content/browser/indexed_db/indexed_db_context_impl.h"
+#include "content/browser/indexed_db/indexed_db_database_callbacks.h"
+#include "content/browser/indexed_db/indexed_db_factory.h"
+#include "content/browser/indexed_db/indexed_db_pending_connection.h"
+#include "content/browser/indexed_db/mock_mojo_indexed_db_callbacks.h"
+#include "content/browser/indexed_db/mock_mojo_indexed_db_database_callbacks.h"
+#include "content/common/indexed_db/indexed_db.mojom.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "mojo/public/cpp/bindings/associated_interface_ptr.h"
+#include "mojo/public/cpp/bindings/strong_associated_binding.h"
+#include "net/url_request/url_request_test_util.h"
+#include "storage/browser/test/mock_quota_manager_proxy.h"
+#include "storage/browser/test/mock_special_storage_policy.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/origin.h"
+
+using indexed_db::mojom::Callbacks;
+using indexed_db::mojom::CallbacksAssociatedPtrInfo;
+using indexed_db::mojom::DatabaseAssociatedPtr;
+using indexed_db::mojom::DatabaseAssociatedRequest;
+using indexed_db::mojom::DatabaseCallbacks;
+using indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo;
+using indexed_db::mojom::FactoryAssociatedPtr;
+using indexed_db::mojom::FactoryAssociatedRequest;
+using indexed_db::mojom::KeyPath;
+using mojo::StrongAssociatedBindingPtr;
+using testing::_;
+
+namespace content {
+namespace {
+
+ACTION_TEMPLATE(MoveArg,
+                HAS_1_TEMPLATE_PARAMS(int, k),
+                AND_1_VALUE_PARAMS(out)) {
+  *out = std::move(*::testing::get<k>(args));
+};
+
+MATCHER_P(IsAssociatedInterfacePtrInfoValid,
+          tf,
+          std::string(negation ? "isn't" : "is") + " " +
+              std::string(tf ? "valid" : "invalid")) {
+  return tf == arg->is_valid();
+}
+
+static const char kDatabaseName[] = "db";
+static const char kOrigin[] = "https://www.example.com";
+static const int kFakeProcessId = 2;
+
+base::FilePath CreateAndReturnTempDir(base::ScopedTempDir* temp_dir) {
+  CHECK(temp_dir->CreateUniqueTempDir());
+  return temp_dir->GetPath();
+}
+
+// Represents a transaction from the renderer side.
+struct TestTransaction {
+  TestTransaction(DatabaseAssociatedPtr* database, int64_t transaction_id)
+      : database(database), transaction_id(transaction_id) {}
+  TestTransaction(TestTransaction&& other) = default;
+
+  void EnqueueCreateObjectStore(int64_t object_store_id,
+                                base::string16 name,
+                                const content::IndexedDBKeyPath& key_path,
+                                bool auto_increment) {
+    (*database)->CreateObjectStore(transaction_id, object_store_id, name,
+                                   key_path, auto_increment);
+  }
+
+  void ExpectTxnComplete(
+      MockMojoIndexedDBDatabaseCallbacks* connection_callbacks) {
+    EXPECT_CALL(*connection_callbacks, Complete(transaction_id)).Times(1);
+  }
+
+  void EnqueueCommit() { (*database)->Commit(transaction_id); }
+
+  DatabaseAssociatedPtr* database;

[Reilly Grant (use Gerrit), 2017/05/11 02:12:01]

It's hard to tell what is going on in the methods on these structs because the
member variable names don't end in an underscore.

[Ken Rockot(use gerrit already), 2017/05/11 17:21:03]

This could be a raw mojom::Database*. No need to hold a raw * to the
InterfacePtr itself unless you're doing something other than dereferencing it
for interface calls.

+  int64_t transaction_id;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestTransaction);
+};
+
+// Represents a database connection from the renderer side.
+struct TestDatabaseConnection {
+  TestDatabaseConnection(url::Origin origin,
+                         base::string16 db_name,
+                         int64_t version,
+                         int64_t upgrade_txn_id)
+      : origin(std::move(origin)),
+        db_name(std::move(db_name)),
+        version(version),
+        upgrade_txn_id(upgrade_txn_id){};
+  TestDatabaseConnection(TestDatabaseConnection&& other) = default;
+
+  ~TestDatabaseConnection() {
+    if (open_callbacks_strong_ptr)
+      open_callbacks_strong_ptr->Close();
+    if (connection_callbacks_strong_ptr)
+      connection_callbacks_strong_ptr->Close();
+  }
+
+  // The factory has to be flushed or the io thread run for the message to be
+  // sent.
+  void QueueOpenRequestAndCreateCallbacks(FactoryAssociatedPtr* factory) {
+    ASSERT_FALSE(connection_openned) << "Connection already opened.";
+    connection_openned = true;
+
+    CallbacksAssociatedPtrInfo callbacks_ptr_info;
+    DatabaseCallbacksAssociatedPtrInfo database_callbacks_ptr_info;
+
+    auto callbacks_request = ::mojo::MakeRequest(&callbacks_ptr_info);
+    auto database_callbacks_request =
+        ::mojo::MakeRequest(&database_callbacks_ptr_info);
+
+    open_callbacks = new testing::StrictMock<MockMojoIndexedDBCallbacks>();
+    connection_callbacks =
+        new testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>();
+
+    open_callbacks_strong_ptr = ::mojo::MakeStrongAssociatedBinding(
+        base::WrapUnique(open_callbacks), std::move(callbacks_request));
+    connection_callbacks_strong_ptr = ::mojo::MakeStrongAssociatedBinding(
+        base::WrapUnique(connection_callbacks),
+        std::move(database_callbacks_request));
+
+    (*factory)->Open(std::move(callbacks_ptr_info),
+                     std::move(database_callbacks_ptr_info), origin, db_name,
+                     version, upgrade_txn_id);

[Reilly Grant (use Gerrit), 2017/05/11 02:12:01]

First, I suggest adding a method to mojo::AssociatedBinding called
CreateInterfacePtrInfoAndBind which does the same thing as
mojo::Binding::CreateInterfacePtrAndBind but works for associated interfaces.

Then, instead of using mojo::MakeStrongAssociatedBinding here I would add a
mojo::AssociatedBinding member to MockMojoIndexedDBCallbacks and add a method
which calls your new CreateInterfacePtrAndBind method to create the ptr_info
arguments to this method.

Then TestDatabaseConnection can own unique_ptrs to the mock callback objects
instead of having the raw pointers you find awkward.

The code would then be:

open_callbacks =
base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>();
connection_callbacks =
base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>();

(*factory)->Open(open_callbacks->CreateInterfacePtrInfoAndBind(),
                 connection_callbacks->CreateInterfacePtrInfoAndBind(),
                 origin, db_name, version, upgrade_txn_id);

[Ken Rockot(use gerrit already), 2017/05/11 17:21:03]

Guess to be safe my reply should have been inline, so just to reiterate what I
replied in email, please don't add another CreateInterfacePtrAndBind method.

+  }
+
+  void ExpectUpgradeCall(int64_t old_version) {
+    EXPECT_FALSE(database_info_.is_valid());
+    EXPECT_CALL(*open_callbacks,
+                MockedUpgradeNeeded(IsAssociatedInterfacePtrInfoValid(true),
+                                    old_version, blink::kWebIDBDataLossNone,
+                                    std::string(""), _))
+        .WillOnce(testing::DoAll(MoveArg<0>(&database_info_),
+                                 testing::SaveArg<4>(&metadata)));
+  }
+
+  void ExpectDatabaseSuccess(bool with_database) {
+    if (with_database) {
+      EXPECT_CALL(
+          *open_callbacks,
+          MockedSuccessDatabase(IsAssociatedInterfacePtrInfoValid(true), _))
+          .WillOnce(testing::DoAll(MoveArg<0>(&database_info_),
+                                   testing::SaveArg<1>(&metadata)));
+    } else {
+      EXPECT_CALL(
+          *open_callbacks,
+          MockedSuccessDatabase(IsAssociatedInterfacePtrInfoValid(false), _));
+    }
+  }
+
+  void ValidateMetadata() {
+    EXPECT_EQ(metadata.name, db_name);
+    EXPECT_EQ(metadata.version, version);
+  }
+
+  void CreateDatabasePtr() {
+    ASSERT_TRUE(database_info_.is_valid());
+    database.Bind(std::move(database_info_));
+  }
+
+  url::Origin origin;
+  base::string16 db_name;
+  int64_t version;
+  int64_t upgrade_txn_id;
+
+  DatabaseAssociatedPtr database;
+  IndexedDBDatabaseMetadata metadata;
+
+  // Callbacks given to the 'open' call - used for upgrading & recieving the db.
+  MockMojoIndexedDBCallbacks* open_callbacks = nullptr;
+  // Callbacks given to the 'open' call for interacting with connection.
+  MockMojoIndexedDBDatabaseCallbacks* connection_callbacks = nullptr;
+
+  bool connection_openned = false;
+  StrongAssociatedBindingPtr<Callbacks> open_callbacks_strong_ptr;

[Ken Rockot(use gerrit already), 2017/05/11 17:21:03]

If you actually have to explicitly close these strong bindings on teardown it
must be because of some expectations the mock classes have. The only side effect
of *not* closing these bindings is that the bound mocks may still receive
message calls after the test fixture is destroyed (which is indeed weird
behavior and you should almost certainly avoid it.)

I would recommend doing what Reilly suggests, with a slight variation. Instead
of using CreateInterfacePtrAndBind, have the constructors take a corresponding
AssociatedInterfaceRequest (e.g. CallbacksAssociatedRequest), and then your code
still simplifies to:

  mojom::CallbacksAssociatedPtrInfo callbacks_ptr_info;
  mojom::DatabaseCallbacksAssociatedPtrInfo database_callbacks_ptr_info;
  open_callbacks_ =
base::MakeUnique<testing::StrictMock<MockMojoIndexedDBCallbacks>>(
      mojo::MakeRequest(&callbacks_ptr_info));
  connection_callbacks_ =
base::MakeUnique<testing::StrictMock<MockMojoIndexedDBDatabaseCallbacks>>(
      mojo::MakeRequest(&database_callbacks_ptr_info));

[dmurph, 2017/05/12 01:22:06]

Thanks!

+  StrongAssociatedBindingPtr<DatabaseCallbacks> connection_callbacks_strong_ptr;
+
+ private:
+  ::indexed_db::mojom::DatabaseAssociatedPtrInfo database_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDatabaseConnection);
+};
+
+}  // namespace
+
+class IndexedDBDispatcherHostTest : public testing::Test {
+ public:
+  IndexedDBDispatcherHostTest()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP),
+        db_task_runner_(new base::TestSimpleTaskRunner()),
+        request_context_getter_(
+            new net::TestURLRequestContextGetter(db_task_runner_)),
+        special_storage_policy_(new MockSpecialStoragePolicy()),
+        quota_manager_proxy_(new MockQuotaManagerProxy(nullptr, nullptr)),
+        context_impl_(
+            new IndexedDBContextImpl(CreateAndReturnTempDir(&temp_dir_),
+                                     special_storage_policy_.get(),
+                                     quota_manager_proxy_.get(),
+                                     db_task_runner_.get())),
+        blob_storage_(ChromeBlobStorageContext::GetFor(&browser_context_)),
+        host_(new IndexedDBDispatcherHost(kFakeProcessId,
+                                          request_context_getter_.get(),
+                                          context_impl_.get(),
+                                          blob_storage_.get())) {
+    base::RunLoop().RunUntilIdle();

[Ken Rockot(use gerrit already), 2017/05/11 17:21:03]

Please never use RunUntilIdle unless you are explicitly testing MessageLoop
behavior and have fine control over all tasks running. This usage effectively
relies on tons of subtle implementation details which are subject to change and
will almost inevitably make your tests hard to maintain. If you need to wait for
a specific event to occur, the proper thing to do is plumb a RunLoop's
QuitClosure() to the right place and have that event run the closure; then you
simply Run() the loop.

[dmurph, 2017/05/12 01:22:06]

I need to do this because I need to run the IO thread from the IDB perspective
(it schedules tasks on the io thread to reply back). What else should I use?

I actually need this one to get the blob storage context initialized, as it
schedules an IO thread task to initialize.

+  }
+
+  void TearDown() override {
+    host_.reset();
+    base::RunLoop().RunUntilIdle();

[Ken Rockot(use gerrit already), 2017/05/11 17:21:03]

Again with the RunUntilIdle() - flakiness danger zone! (apologies if these are
just temporary for the purpose of hacking)

[dmurph, 2017/05/12 01:22:06]

These are temp, above is not though.

+    db_task_runner_->RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
+    // We leak files if this doesn't return true.
+    ASSERT_TRUE(temp_dir_.Delete());
+  }
+
+  void SetUp() override {
+    FactoryAssociatedRequest request =
+        ::mojo::MakeIsolatedRequest(&idb_mojo_factory_);
+    host_->AddBinding(std::move(request));
+  }
+
+ protected:
+  TestBrowserThreadBundle thread_bundle_;
+  TestBrowserContext browser_context_;
+
+  base::ScopedTempDir temp_dir_;
+  scoped_refptr<base::TestSimpleTaskRunner> db_task_runner_;
+  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+  scoped_refptr<MockSpecialStoragePolicy> special_storage_policy_;
+  scoped_refptr<MockQuotaManagerProxy> quota_manager_proxy_;
+  scoped_refptr<IndexedDBContextImpl> context_impl_;
+  scoped_refptr<ChromeBlobStorageContext> blob_storage_;
+  std::unique_ptr<IndexedDBDispatcherHost, BrowserThread::DeleteOnIOThread>
+      host_;
+  FactoryAssociatedPtr idb_mojo_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(IndexedDBDispatcherHostTest);
+};
+
+TEST_F(IndexedDBDispatcherHostTest, TestCloseConnectionBeforeUpgrade) {
+  TestDatabaseConnection connection(url::Origin(GURL(kOrigin)),
+                                    base::UTF8ToUTF16(kDatabaseName), 1, 1);
+
+  // Queue open request message.
+  connection.QueueOpenRequestAndCreateCallbacks(&idb_mojo_factory_);
+
+  // Create mock expectation.
+  connection.ExpectUpgradeCall(IndexedDBDatabaseMetadata::NO_VERSION);
+
+  // Send message.
+  idb_mojo_factory_.FlushForTesting();
+
+  // Run database tasks and io tasks to send message back.
+  db_task_runner_->RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+
+  connection.ValidateMetadata();
+}
+
+TEST_F(IndexedDBDispatcherHostTest, TestOpenConnectionAndUpgrade) {
+  TestDatabaseConnection connection(url::Origin(GURL(kOrigin)),
+                                    base::UTF8ToUTF16(kDatabaseName), 1, 1);
+
+  // Open DB.
+  connection.QueueOpenRequestAndCreateCallbacks(&idb_mojo_factory_);
+  connection.ExpectUpgradeCall(IndexedDBDatabaseMetadata::NO_VERSION);
+  idb_mojo_factory_.FlushForTesting();
+  db_task_runner_->RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+  connection.ValidateMetadata();
+  connection.CreateDatabasePtr();
+
+  // Create object store.
+  TestTransaction txn(&connection.database, 1);
+  txn.EnqueueCreateObjectStore(10, base::UTF8ToUTF16(kDatabaseName),
+                               content::IndexedDBKeyPath(), false);
+  txn.EnqueueCommit();
+  txn.ExpectTxnComplete(connection.connection_callbacks);
+  connection.ExpectDatabaseSuccess(/* with_database */ false);
+
+  connection.database.FlushForTesting();
+  db_task_runner_->RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+}
+
+}  // namespace content