From 3a76ec1999dc11db01da59a322b0b9f2eceba3b2 Mon Sep 17 00:00:00 2001 From: "tim@chromium.org" Date: Thu, 24 Sep 2009 20:24:48 +0000 Subject: Use chrome/base synchronization primitives and threads instead of pthreads in SyncerThread. The old pthread impl can be used by specifying --syncer-thread-pthreads for comparison until we settle fully on the final impl (I have a MessageLoop-based impl in progress). The default SyncerThread is as close to the pthreads-impl semantics as I could get, with one exception: it does not offer a time-out when calling Stop(), because it greatly simplifies the implementation. I first implemented it *with* the timeout, and for sake of experimentation while this is in shuffle I am checking it in as SyncerThreadTimedStop, available by using --syncer-thread-timed-stop. I'm not sure which we want ultimately, but it's useful to have around when building the MessageLoop based impl. I had to refactor the interface slightly to allow multiple implementations, I think it will be quite useful while working on the MessageLoop impl. Added several tests to SyncerThreadUnittest, which all impls now pass ( just pass the command line flag to try each out). TEST=SyncerThreadTest, SyncerThreadWithSyncerTest, integration tests Review URL: http://codereview.chromium.org/214033 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27117 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/test/sync/engine/mock_server_connection.cc | 22 ++++++++++++++++++---- chrome/test/sync/engine/mock_server_connection.h | 9 ++++++++- .../sync/engine/test_directory_setter_upper.cc | 20 +++++++++++++++++++- .../test/sync/engine/test_directory_setter_upper.h | 22 +++++++++++++++++++--- 4 files changed, 64 insertions(+), 9 deletions(-) (limited to 'chrome/test/sync') diff --git a/chrome/test/sync/engine/mock_server_connection.cc b/chrome/test/sync/engine/mock_server_connection.cc index bd3d90f..c4595bd 100644 --- a/chrome/test/sync/engine/mock_server_connection.cc +++ b/chrome/test/sync/engine/mock_server_connection.cc @@ -25,6 +25,7 @@ using sync_pb::CommitResponse_EntryResponse; using sync_pb::GetUpdatesMessage; using sync_pb::SyncEntity; using syncable::DirectoryManager; +using syncable::ScopedDirLookup; using syncable::WriteTransaction; MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr, @@ -38,12 +39,13 @@ MockConnectionManager::MockConnectionManager(DirectoryManager* dirmgr, client_stuck_(false), commit_time_rename_prepended_string_(""), fail_next_postbuffer_(false), - directory_(dirmgr, name), + directory_manager_(dirmgr), + directory_name_(name), mid_commit_callback_function_(NULL), + mid_commit_observer_(NULL), client_command_(NULL), next_position_in_parent_(2) { server_reachable_ = true; - CHECK(directory_.good()); }; MockConnectionManager::~MockConnectionManager() { @@ -60,9 +62,18 @@ void MockConnectionManager::SetMidCommitCallbackFunction( mid_commit_callback_function_ = callback; } +void MockConnectionManager::SetMidCommitObserver( + MockConnectionManager::MidCommitObserver* observer) { + mid_commit_observer_ = observer; +} + bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params, const string& path, const string& auth_token) { + + ScopedDirLookup directory(directory_manager_, directory_name_); + CHECK(directory.good()); + ClientToServerMessage post; CHECK(post.ParseFromString(params->buffer_in)); client_stuck_ = post.sync_problem_detected(); @@ -73,7 +84,7 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params, // network. As we can't test this we do the next best thing and hang here // when there's an issue. { - WriteTransaction wt(directory_, syncable::UNITTEST, __FILE__, __LINE__); + WriteTransaction wt(directory, syncable::UNITTEST, __FILE__, __LINE__); } if (fail_next_postbuffer_) { fail_next_postbuffer_ = false; @@ -105,9 +116,12 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params, } response.SerializeToString(params->buffer_out); if (mid_commit_callback_function_) { - if (mid_commit_callback_function_(directory_)) + if (mid_commit_callback_function_(directory)) mid_commit_callback_function_ = 0; } + if (mid_commit_observer_) { + mid_commit_observer_->Observe(); + } return result; } diff --git a/chrome/test/sync/engine/mock_server_connection.h b/chrome/test/sync/engine/mock_server_connection.h index e5573e0..8c0cca7 100644 --- a/chrome/test/sync/engine/mock_server_connection.h +++ b/chrome/test/sync/engine/mock_server_connection.h @@ -31,6 +31,10 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { // activity would normally take place. This aids simulation of race // conditions. typedef bool (*TestCallbackFunction)(syncable::Directory* dir); + class MidCommitObserver { + public: + virtual void Observe() = 0; + }; MockConnectionManager(syncable::DirectoryManager* dirmgr, PathString name); virtual ~MockConnectionManager(); @@ -45,6 +49,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { // Control of commit response. void SetMidCommitCallbackFunction(TestCallbackFunction callback); + void SetMidCommitObserver(MidCommitObserver* observer); // Set this if you want commit to perform commit time rename. Will request // that the client renames all commited entries, prepending this string. @@ -177,11 +182,13 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager { bool fail_next_postbuffer_; // Our directory. - syncable::ScopedDirLookup directory_; + syncable::DirectoryManager* directory_manager_; + PathString directory_name_; // The updates we'll return to the next request. sync_pb::GetUpdatesResponse updates_; TestCallbackFunction mid_commit_callback_function_; + MidCommitObserver* mid_commit_observer_; scoped_ptr client_command_; diff --git a/chrome/test/sync/engine/test_directory_setter_upper.cc b/chrome/test/sync/engine/test_directory_setter_upper.cc index 468875a..0a7b32d 100644 --- a/chrome/test/sync/engine/test_directory_setter_upper.cc +++ b/chrome/test/sync/engine/test_directory_setter_upper.cc @@ -20,12 +20,15 @@ TestDirectorySetterUpper::TestDirectorySetterUpper() : name_(PSTR("Test")) {} TestDirectorySetterUpper::~TestDirectorySetterUpper() {} -void TestDirectorySetterUpper::SetUp() { +void TestDirectorySetterUpper::Init() { PathString test_data_dir_ = PSTR("."); manager_.reset(new DirectoryManager(test_data_dir_)); file_path_ = manager_->GetSyncDataDatabasePath(); PathRemove(file_path_.c_str()); +} +void TestDirectorySetterUpper::SetUp() { + Init(); ASSERT_TRUE(manager()->Open(name())); } @@ -64,4 +67,19 @@ void TestDirectorySetterUpper::RunInvariantCheck(const ScopedDirLookup& dir) { } } +void ManuallyOpenedTestDirectorySetterUpper::SetUp() { + Init(); +} + +void ManuallyOpenedTestDirectorySetterUpper::Open() { + ASSERT_TRUE(manager()->Open(name())); + was_opened_ = true; +} + +void ManuallyOpenedTestDirectorySetterUpper::TearDown() { + if (was_opened_) { + TestDirectorySetterUpper::TearDown(); + } +} + } // namespace browser_sync diff --git a/chrome/test/sync/engine/test_directory_setter_upper.h b/chrome/test/sync/engine/test_directory_setter_upper.h index 0cac7e1..0cfa3e6 100644 --- a/chrome/test/sync/engine/test_directory_setter_upper.h +++ b/chrome/test/sync/engine/test_directory_setter_upper.h @@ -44,20 +44,23 @@ namespace browser_sync { class TestDirectorySetterUpper { public: TestDirectorySetterUpper(); - ~TestDirectorySetterUpper(); + virtual ~TestDirectorySetterUpper(); // Create a DirectoryManager instance and use it to open the directory. // Clears any existing database backing files that might exist on disk. - void SetUp(); + virtual void SetUp(); // Undo everything done by SetUp(): close the directory and delete the // backing files. Before closing the directory, this will run the directory // invariant checks and perform the SaveChanges action on the directory. - void TearDown(); + virtual void TearDown(); syncable::DirectoryManager* manager() const { return manager_.get(); } const PathString& name() const { return name_; } + protected: + virtual void Init(); + private: void RunInvariantCheck(const syncable::ScopedDirLookup& dir); @@ -66,6 +69,19 @@ class TestDirectorySetterUpper { PathString file_path_; }; +// A variant of the above where SetUp does not actually open the directory. +// You must manually invoke Open(). This is useful if you are writing a test +// that depends on the DirectoryManager::OPENED event. +class ManuallyOpenedTestDirectorySetterUpper : public TestDirectorySetterUpper { + public: + ManuallyOpenedTestDirectorySetterUpper() : was_opened_(false) {} + virtual void SetUp(); + virtual void TearDown(); + void Open(); + private: + bool was_opened_; +}; + } // namespace browser_sync #endif // CHROME_TEST_SYNC_ENGINE_TEST_DIRECTORY_SETTER_UPPER_H_ -- cgit v1.1