Revert 79902 - [Sync] Add TestUserShare class meant to be used by syncapi-related unit tests

BUG=
TEST=


Review URL: http://codereview.chromium.org/6759041

TBR=akalin@chromium.org
Review URL: http://codereview.chromium.org/6728006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79904 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 57 insertions, 184 deletions

diff --git a/chrome/browser/sync/engine/syncapi_unittest.cc b/chrome/browser/sync/engine/syncapi_unittest.cc
index 1cc5032..28d8c98 100644
--- a/chrome/browser/sync/engine/syncapi_unittest.cc
+++ b/chrome/browser/sync/engine/syncapi_unittest.cc
@@ -34,7 +34,7 @@
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/syncable/syncable_id.h"
 #include "chrome/browser/sync/util/cryptographer.h"
-#include "chrome/test/sync/engine/test_user_share.h"
+#include "chrome/test/sync/engine/test_directory_setter_upper.h"
 #include "chrome/test/values_test_util.h"
 #include "content/browser/browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -166,29 +166,34 @@ int64 MakeServerNodeForType(UserShare* share,
 class SyncApiTest : public testing::Test {
  public:
   virtual void SetUp() {
-    test_user_share_.SetUp();
+    setter_upper_.SetUp();
+    share_.dir_manager.reset(setter_upper_.manager());
+    share_.name = setter_upper_.name();
   }
 
   virtual void TearDown() {
-    test_user_share_.TearDown();
+    // |share_.dir_manager| does not actually own its value.
+    ignore_result(share_.dir_manager.release());
+    setter_upper_.TearDown();
   }
 
  protected:
-  browser_sync::TestUserShare test_user_share_;
+  UserShare share_;
+  browser_sync::TestDirectorySetterUpper setter_upper_;
 };
 
 TEST_F(SyncApiTest, SanityCheckTest) {
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     EXPECT_TRUE(trans.GetWrappedTrans() != NULL);
   }
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     EXPECT_TRUE(trans.GetWrappedTrans() != NULL);
   }
   {
     // No entries but root should exist
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     // Metahandle 1 can be root, sanity check 2
     EXPECT_FALSE(node.InitByIdLookup(2));
@@ -197,17 +202,16 @@ TEST_F(SyncApiTest, SanityCheckTest) {
 
 TEST_F(SyncApiTest, BasicTagWrite) {
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
     EXPECT_EQ(root_node.GetFirstChildId(), 0);
   }
 
-  ignore_result(MakeNode(test_user_share_.user_share(),
-                         syncable::BOOKMARKS, "testtag"));
+  ignore_result(MakeNode(&share_, syncable::BOOKMARKS, "testtag"));
 
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
@@ -237,21 +241,18 @@ TEST_F(SyncApiTest, GenerateSyncableHash) {
 
 TEST_F(SyncApiTest, ModelTypesSiloed) {
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
     EXPECT_EQ(root_node.GetFirstChildId(), 0);
   }
 
-  ignore_result(MakeNode(test_user_share_.user_share(),
-                         syncable::BOOKMARKS, "collideme"));
-  ignore_result(MakeNode(test_user_share_.user_share(),
-                         syncable::PREFERENCES, "collideme"));
-  ignore_result(MakeNode(test_user_share_.user_share(),
-                         syncable::AUTOFILL, "collideme"));
+  ignore_result(MakeNode(&share_, syncable::BOOKMARKS, "collideme"));
+  ignore_result(MakeNode(&share_, syncable::PREFERENCES, "collideme"));
+  ignore_result(MakeNode(&share_, syncable::AUTOFILL, "collideme"));
 
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
 
     ReadNode bookmarknode(&trans);
     EXPECT_TRUE(bookmarknode.InitByClientTagLookup(syncable::BOOKMARKS,
@@ -273,13 +274,13 @@ TEST_F(SyncApiTest, ModelTypesSiloed) {
 
 TEST_F(SyncApiTest, ReadMissingTagsFails) {
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     EXPECT_FALSE(node.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
   }
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     WriteNode node(&trans);
     EXPECT_FALSE(node.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
@@ -294,7 +295,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
   std::wstring test_title(L"test1");
 
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
 
@@ -315,7 +316,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
 
   // Ensure we can delete something with a tag.
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     WriteNode wnode(&trans);
     EXPECT_TRUE(wnode.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
@@ -328,7 +329,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
   // Lookup of a node which was deleted should return failure,
   // but have found some data about the node.
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     EXPECT_FALSE(node.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
@@ -338,7 +339,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
   }
 
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     ReadNode folder_node(&trans);
     EXPECT_TRUE(folder_node.InitByIdLookup(folder_id));
 
@@ -355,7 +356,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
 
   // Now look up should work.
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS,
         "testtag"));
@@ -367,11 +368,11 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
 TEST_F(SyncApiTest, WriteAndReadPassword) {
   KeyParams params = {"localhost", "username", "passphrase"};
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     trans.GetCryptographer()->AddKey(params);
   }
   {
-    WriteTransaction trans(test_user_share_.user_share());
+    WriteTransaction trans(&share_);
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
 
@@ -383,7 +384,7 @@ TEST_F(SyncApiTest, WriteAndReadPassword) {
     password_node.SetPasswordSpecifics(data);
   }
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
 
@@ -440,7 +441,7 @@ void CheckNodeValue(const BaseNode& node, const DictionaryValue& value) {
 }  // namespace
 
 TEST_F(SyncApiTest, BaseNodeToValue) {
-  ReadTransaction trans(test_user_share_.user_share());
+  ReadTransaction trans(&share_);
   ReadNode node(&trans);
   node.InitByRootLookup();
   scoped_ptr<DictionaryValue> value(node.ToValue());
@@ -520,11 +521,10 @@ class MockExtraChangeRecordData
 }  // namespace
 
 TEST_F(SyncApiTest, ChangeRecordToValue) {
-  int64 child_id = MakeNode(test_user_share_.user_share(),
-                            syncable::BOOKMARKS, "testtag");
+  int64 child_id = MakeNode(&share_, syncable::BOOKMARKS, "testtag");
   sync_pb::EntitySpecifics child_specifics;
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     ReadNode node(&trans);
     EXPECT_TRUE(node.InitByIdLookup(child_id));
     child_specifics = node.GetEntry()->Get(syncable::SPECIFICS);
@@ -532,7 +532,7 @@ TEST_F(SyncApiTest, ChangeRecordToValue) {
 
   // Add
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     SyncManager::ChangeRecord record;
     record.action = SyncManager::ChangeRecord::ACTION_ADD;
     record.id = 1;
@@ -544,7 +544,7 @@ TEST_F(SyncApiTest, ChangeRecordToValue) {
 
   // Update
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     SyncManager::ChangeRecord record;
     record.action = SyncManager::ChangeRecord::ACTION_UPDATE;
     record.id = child_id;
@@ -556,7 +556,7 @@ TEST_F(SyncApiTest, ChangeRecordToValue) {
 
   // Delete (no extra)
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     SyncManager::ChangeRecord record;
     record.action = SyncManager::ChangeRecord::ACTION_DELETE;
     record.id = child_id + 1;
@@ -567,7 +567,7 @@ TEST_F(SyncApiTest, ChangeRecordToValue) {
 
   // Delete (with extra)
   {
-    ReadTransaction trans(test_user_share_.user_share());
+    ReadTransaction trans(&share_);
     SyncManager::ChangeRecord record;
     record.action = SyncManager::ChangeRecord::ACTION_DELETE;
     record.id = child_id + 1;
diff --git a/chrome/browser/sync/js_sync_manager_observer_unittest.cc b/chrome/browser/sync/js_sync_manager_observer_unittest.cc
index b9c0c33..f5ed961 100644
--- a/chrome/browser/sync/js_sync_manager_observer_unittest.cc
+++ b/chrome/browser/sync/js_sync_manager_observer_unittest.cc
@@ -13,7 +13,7 @@
 #include "chrome/browser/sync/js_test_util.h"
 #include "chrome/browser/sync/sessions/session_state.h"
 #include "chrome/browser/sync/syncable/model_type.h"
-#include "chrome/test/sync/engine/test_user_share.h"
+#include "chrome/test/sync/engine/test_directory_setter_upper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace browser_sync {
@@ -194,8 +194,11 @@ int64 MakeNode(sync_api::UserShare* share, syncable::ModelType model_type) {
 TEST_F(JsSyncManagerObserverTest, OnChangesApplied) {
   InSequence dummy;
 
-  TestUserShare test_user_share;
-  test_user_share.SetUp();
+  TestDirectorySetterUpper setter_upper;
+  sync_api::UserShare share;
+  setter_upper.SetUp();
+  share.dir_manager.reset(setter_upper.manager());
+  share.name = setter_upper.name();
 
   // We don't test with passwords as that requires additional setup.
 
@@ -203,8 +206,7 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) {
   sync_api::SyncManager::ChangeRecord changes[syncable::MODEL_TYPE_COUNT];
   for (int i = syncable::AUTOFILL_PROFILE;
        i < syncable::MODEL_TYPE_COUNT; ++i) {
-    changes[i].id =
-        MakeNode(test_user_share.user_share(), syncable::ModelTypeFromInt(i));
+    changes[i].id = MakeNode(&share, syncable::ModelTypeFromInt(i));
     switch (i % 3) {
       case 0:
         changes[i].action =
@@ -220,7 +222,7 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) {
         break;
     }
     {
-      sync_api::ReadTransaction trans(test_user_share.user_share());
+      sync_api::ReadTransaction trans(&share);
       sync_api::ReadNode node(&trans);
       EXPECT_TRUE(node.InitByIdLookup(changes[i].id));
       changes[i].specifics = node.GetEntry()->Get(syncable::SPECIFICS);
@@ -241,7 +243,7 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) {
     ListValue* expected_changes = new ListValue();
     expected_args.Append(expected_changes);
     for (int j = i; j < syncable::MODEL_TYPE_COUNT; ++j) {
-      sync_api::ReadTransaction trans(test_user_share.user_share());
+      sync_api::ReadTransaction trans(&share);
       expected_changes->Append(changes[j].ToValue(&trans));
     }
     EXPECT_CALL(mock_router_,
@@ -252,13 +254,15 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) {
   // Fire OnChangesApplied() for each data type.
   for (int i = syncable::AUTOFILL_PROFILE;
        i < syncable::MODEL_TYPE_COUNT; ++i) {
-    sync_api::ReadTransaction trans(test_user_share.user_share());
+    sync_api::ReadTransaction trans(&share);
     sync_manager_observer_.OnChangesApplied(syncable::ModelTypeFromInt(i),
                                             &trans, &changes[i],
                                             syncable::MODEL_TYPE_COUNT - i);
   }
 
-  test_user_share.TearDown();
+  // |share.dir_manager| does not actually own its value.
+  ignore_result(share.dir_manager.release());
+  setter_upper.TearDown();
 }
 
 }  // namespace
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 3df2967..0dfd105 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -254,38 +254,12 @@
         '../testing/gtest.gyp:gtest',
         'sync',
       ],
-      'export_dependent_settings': [
-        '../testing/gmock.gyp:gmock',
-        '../testing/gtest.gyp:gtest',
-        'sync',
-      ],
       'include_dirs': [
         '..',
       ],
       'sources': [
         'browser/sync/js_test_util.cc',
         'browser/sync/js_test_util.h',
-        'test/sync/engine/test_directory_setter_upper.cc',
-        'test/sync/engine/test_directory_setter_upper.h',
-      ],
-    },
-    {
-      'target_name': 'test_support_syncapi',
-      'type': '<(library)',
-      'dependencies': [
-        'syncapi',
-        'test_support_sync',
-      ],
-      'export_dependent_settings': [
-        'syncapi',
-        'test_support_sync',
-      ],
-      'include_dirs': [
-        '..',
-      ],
-      'sources': [
-        'test/sync/engine/test_user_share.cc',
-        'test/sync/engine/test_user_share.h',
       ],
     },
     {
@@ -1142,7 +1116,6 @@
         'service',
         'test_support_common',
         'test_support_sync',
-        'test_support_syncapi',
         'test_support_unit',
         'utility',
         '../app/app.gyp:app_base',
@@ -2939,6 +2912,8 @@
         'test/sync/engine/mock_gaia_authenticator.h',
         'test/sync/engine/mock_gaia_authenticator_unittest.cc',
         'test/sync/engine/syncer_command_test.h',
+        'test/sync/engine/test_directory_setter_upper.cc',
+        'test/sync/engine/test_directory_setter_upper.h',
         'test/sync/engine/test_id_factory.h',
         'test/sync/engine/test_syncable_utils.cc',
         'test/sync/engine/test_syncable_utils.h',
@@ -2968,7 +2943,6 @@
         'sync_notifier',
         'test_support_common',
         'test_support_sync',
-        'test_support_syncapi',
         'test_support_sync_notifier',
         'test_support_unit',
       ],
diff --git a/chrome/test/sync/engine/test_directory_setter_upper.cc b/chrome/test/sync/engine/test_directory_setter_upper.cc
index c124d52..a0e4431 100644
--- a/chrome/test/sync/engine/test_directory_setter_upper.cc
+++ b/chrome/test/sync/engine/test_directory_setter_upper.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/test/sync/engine/test_directory_setter_upper.h"
 
-#include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/string_util.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
@@ -27,9 +26,8 @@ TestDirectorySetterUpper::~TestDirectorySetterUpper() {}
 void TestDirectorySetterUpper::Init() {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   reset_directory_manager(new DirectoryManager(temp_dir_.path()));
-  // There shouldn't be any existing database in the newly-created
-  // temp dir.
-  ASSERT_FALSE(file_util::PathExists(manager_->GetSyncDataDatabasePath()));
+  file_path_ = manager_->GetSyncDataDatabasePath();
+  file_util::Delete(file_path_, false);
 }
 
 void TestDirectorySetterUpper::reset_directory_manager(DirectoryManager* d) {
@@ -59,7 +57,8 @@ void TestDirectorySetterUpper::TearDown() {
   manager()->FinalSaveChangesForAll();
   manager()->Close(name());
   manager_.reset();
-  ASSERT_TRUE(temp_dir_.Delete());
+  EXPECT_TRUE(file_util::Delete(file_path_, false));
+  file_path_ = FilePath(FILE_PATH_LITERAL(""));
 }
 
 void TestDirectorySetterUpper::RunInvariantCheck(const ScopedDirLookup& dir) {
diff --git a/chrome/test/sync/engine/test_directory_setter_upper.h b/chrome/test/sync/engine/test_directory_setter_upper.h
index 75506c0..0621301 100644
--- a/chrome/test/sync/engine/test_directory_setter_upper.h
+++ b/chrome/test/sync/engine/test_directory_setter_upper.h
@@ -33,7 +33,6 @@
 
 #include <string>
 
-#include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_temp_dir.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
@@ -75,9 +74,8 @@ class TestDirectorySetterUpper {
 
   scoped_ptr<syncable::DirectoryManager> manager_;
   const std::string name_;
+  FilePath file_path_;
   ScopedTempDir temp_dir_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestDirectorySetterUpper);
 };
 
 // A variant of the above where SetUp does not actually open the directory.
diff --git a/chrome/test/sync/engine/test_user_share.cc b/chrome/test/sync/engine/test_user_share.cc
deleted file mode 100644
index 2fdd36f..0000000
--- a/chrome/test/sync/engine/test_user_share.cc
+++ /dev/null
@@ -1,35 +0,0 @@
-// Copyright (c) 2011 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 "chrome/test/sync/engine/test_user_share.h"
-
-#include "base/compiler_specific.h"
-
-namespace browser_sync {
-
-TestUserShare::TestUserShare() {}
-
-TestUserShare::~TestUserShare() {
-  CHECK(!user_share_.dir_manager.get());
-}
-
-void TestUserShare::SetUp() {
-  setter_upper_.SetUp();
-  // HACK: We have two scoped_ptrs to the same object.  But we make
-  // sure to release one in TearDown.
-  user_share_.dir_manager.reset(setter_upper_.manager());
-  user_share_.name = setter_upper_.name();
-}
-
-void TestUserShare::TearDown() {
-  // Make sure we don't free the manager twice.
-  ignore_result(user_share_.dir_manager.release());
-  setter_upper_.TearDown();
-}
-
-sync_api::UserShare* TestUserShare::user_share() {
-  return &user_share_;
-}
-
-}  // namespace browser_sync
diff --git a/chrome/test/sync/engine/test_user_share.h b/chrome/test/sync/engine/test_user_share.h
deleted file mode 100644
index 00d73d1..0000000
--- a/chrome/test/sync/engine/test_user_share.h
+++ /dev/null
@@ -1,67 +0,0 @@
-// Copyright (c) 2011 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.
-//
-// A handy class that takes care of setting up and destroying a
-// sync_api::UserShare instance for unit tests that require one.
-//
-// The expected usage is to make this a component of your test fixture:
-//
-//   class AwesomenessTest : public testing::Test {
-//    public:
-//     virtual void SetUp() {
-//       test_user_share_.SetUp();
-//     }
-//     virtual void TearDown() {
-//       test_user_share_.TearDown();
-//     }
-//    protected:
-//     TestUserShare user_share_;
-//   };
-//
-// Then, in your tests:
-//
-//   TEST_F(AwesomenessTest, IsMaximal) {
-//     sync_api::ReadTransaction trans(test_user_share_.user_share());
-//     ...
-//   }
-//
-
-#ifndef CHROME_TEST_SYNC_ENGINE_TEST_USER_SHARE_H_
-#define CHROME_TEST_SYNC_ENGINE_TEST_USER_SHARE_H_
-#pragma once
-
-#include "base/basictypes.h"
-#include "chrome/browser/sync/engine/syncapi.h"
-#include "chrome/test/sync/engine/test_directory_setter_upper.h"
-
-namespace browser_sync {
-
-class TestUserShare {
- public:
-  TestUserShare();
-  ~TestUserShare();
-
-  // Sets up the UserShare instance.  Clears any existing database
-  // backing files that might exist on disk.
-  void SetUp();
-
-  // Undo everything done by SetUp(): closes the UserShare and deletes
-  // the backing files. Before closing the directory, this will run
-  // the directory invariant checks and perform the SaveChanges action
-  // on the user share's directory.
-  void TearDown();
-
-  // Non-NULL iff called between a call to SetUp() and TearDown().
-  sync_api::UserShare* user_share();
-
- private:
-  TestDirectorySetterUpper setter_upper_;
-  sync_api::UserShare user_share_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestUserShare);
-};
-
-}  // namespace browser_sync
-
-#endif  // CHROME_TEST_SYNC_ENGINE_TEST_USER_SHARE_H_