wifi_sync: implement ACTION_ADD in WifiCredentialSyncableService

components/wifi_sync/wifi_credential_syncable_service_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.

[erikwright (departed), 2015/01/13 19:21:59]

2015

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/wifi_sync/wifi_credential_syncable_service.h"
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/tracked_objects.h"
+#include "components/wifi_sync/fake_wifi_config_delegate.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "components/wifi_sync/wifi_security_class.h"
+#include "sync/api/fake_sync_change_processor.h"
+#include "sync/api/sync_change.h"
+#include "sync/api/sync_error.h"
+#include "sync/api/sync_error_factory_mock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi_sync {
+
+using syncer::FakeSyncChangeProcessor;
+using syncer::SyncErrorFactoryMock;
+
+// Unit tests for WifiCredentialSyncableService.
+class WifiCredentialSyncableServiceTest : public testing::Test {
+ protected:
+  WifiCredentialSyncableServiceTest()
+      : change_processor_(nullptr) {
+    auto config_delegate = make_scoped_ptr(new FakeWifiConfigDelegate());

[erikwright (departed), 2015/01/13 19:21:59]

not a case for auto.

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+    config_delegate_ = config_delegate.get();

[erikwright (departed), 2015/01/13 19:21:59]

I'm not sure if there's a hard convention or rule, but I would think that all
primitive members should be initialized in the initializer list even if they are
definitely assigned in the constructor body.

[stevenjb, 2015/01/14 00:12:49]

That is correct, config_delegate_ should be initialized to nullptr in the
constructor.
That said, I think this is awkward at best. I would either add a
config_delegate_for_test() accessor to WifiCredentialSyncableService, or remove
the local scoped ptr and just assign 'new FakeWifiConfigDelegate()' directly to
config_delegate_ with a comment that ownership is passed to syncable_service_.

[erikwright (departed), 2015/01/14 14:59:27]

This pattern is common and reasonable in unit tests. There is no reason to
complicate the public API of the class under test.

An alternative approach is to split the mock object in two. You create a data
object (in this case, just an int) and pass a pointer to that in the mock's
constructor. Then you pass the mock to the class under test and use the data
object to verify the behaviour.

For example:

int call_count = 0;  // really, should be a member and initialized in
initializer list
syncable_service_.reset(new WifiCredentialSyncableService(make_scoped_ptr(new
FakeWifiConfigDelegate(&call_count))));

Naturally, if the state of the mock becomes more complex you would introduce a
struct or class to manage the data instead of a primitive.

I have three different approaches, depending on the complexity of the situation:

(1) As coded in this patchset, when the lifetime of the mock object is
well-known (i.e. I can safely avoid using my raw pointer after the class under
test destroys it)
(2) As above in this comment, if I can at least guarantee that the shared data
object (or primitive) will outlive the mock, or if multiple mock instances need
to collect data together.
(3) Similar to (2), but refcounting the data object, if the lifetime of the mock
is obscured somehow and might otherwise outlive the data object.

[mukesh agrawal, 2015/01/20 21:13:15]

I've moved the allocation of FakeWifiConfigDelegate into the initializer list.
This takes care of initialize the field in the initializer list, while making
the overall code slightly simpler. Since we don't allow exceptions, this code
should still guarantee that we don't leak the Fake.

+    syncable_service_.reset(
+        new WifiCredentialSyncableService(config_delegate.Pass()));
+  }
+
+  // Wrappers for methods in WifiCredentialSyncableService. Sorted by
+  // the declaration order of the WifiCredentialSyncableService
+  // methods they call.

[stevenjb, 2015/01/14 00:12:49]

I don't think noting the sorting order of these methods is particularly helpful.

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+  syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& caller_location,
+      const syncer::SyncChangeList& change_list) {
+    return syncable_service_->ProcessSyncChanges(caller_location, change_list);
+  }

[stevenjb, 2015/01/14 00:12:49]

WS

[mukesh agrawal, 2015/01/20 21:13:15]

I believe this case is correct. AddToSyncedNetworks is also covered by the "//
comment" at line 35, so no whitespace is warranted.

Let me know if I've misunderstood.

+  bool AddToSyncedNetworks(
+      const std::string& item_id, const WifiCredential& credential) {
+    return syncable_service_->AddToSyncedNetworks(item_id, credential);
+  }
+
+  // Returns the number of change requests received by
+  // |change_processor_|.
+  int GetChangeProcessorChangeCount() {
+    CHECK(change_processor_);
+    return change_processor_->changes().size();
+  }

[stevenjb, 2015/01/14 00:12:49]

WS

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+  // Returns the number of times AddToLocalNetworks has been called on
+  // |config_delegate_|.
+  int GetWifiDelegateAddCount() {
+    return config_delegate_->GetAddCount();
+  }

[stevenjb, 2015/01/14 00:12:49]

WS

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+  // Returns a new WifiCredential constructed from the given parameters.
+  WifiCredential MakeCredential(const std::string& ssid,
+                                const WifiSecurityClass security_class,
+                                const std::string& passphrase) {
+    scoped_ptr<WifiCredential> credential =
+        WifiCredential::Create(
+            WifiCredential::MakeSsidBytesForTest(ssid),
+            security_class,
+            passphrase);
+    CHECK(credential);
+    return *credential;
+  }

[stevenjb, 2015/01/14 00:12:49]

WS

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+  // Starts |syncable_service_|, providing the service an empty
+  // initial data list.
+  void StartSyncing() {
+    auto change_processor = make_scoped_ptr(new FakeSyncChangeProcessor());
+    change_processor_ = change_processor.get();
+    syncable_service_->MergeDataAndStartSyncing(
+        syncer::WIFI_CREDENTIALS,
+        syncer::SyncDataList(),
+        change_processor.Pass(),
+        make_scoped_ptr(new SyncErrorFactoryMock()));
+  }
+
+ private:
+  scoped_ptr<WifiCredentialSyncableService> syncable_service_;
+  FakeWifiConfigDelegate* config_delegate_;  // Owned by |syncable_service_|
+  FakeSyncChangeProcessor* change_processor_;  // Owned by |syncable_service_|
+
+  DISALLOW_COPY_AND_ASSIGN(WifiCredentialSyncableServiceTest);
+};
+
+TEST_F(WifiCredentialSyncableServiceTest, ProcessSyncChangesNotStarted) {
+  auto sync_error(ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList()));

[erikwright (departed), 2015/01/13 19:21:59]

not a case for auto, here and below

[mukesh agrawal, 2015/01/20 21:13:15]

Done.

+  ASSERT_TRUE(sync_error.IsSet());
+  EXPECT_EQ(syncer::SyncError::UNREADY_ERROR, sync_error.error_type());
+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest, ProcessSyncChangesInvalidChange) {
+  StartSyncing();
+  auto sync_error(
+      ProcessSyncChanges(FROM_HERE,
+                         syncer::SyncChangeList(1, syncer::SyncChange())));
+  ASSERT_TRUE(sync_error.IsSet());
+  EXPECT_EQ(syncer::SyncError::DATATYPE_ERROR, sync_error.error_type());
+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+// The success cases for ProcessSyncChanges will be tested as part of

[erikwright (departed), 2015/01/13 19:21:59]

Is this consistent with other syncable services?

[mukesh agrawal, 2015/01/20 21:13:15]

Thanks for asking. Taking a closer look at some of the other SyncableServices,
it turns out that there is a solution for the problem I ran into. (I needed the
AttachmentServiceProxyForTest.)

+// the sync integration tests. (We don't test the success cases here,
+// due to the difficulty of creating proper SyncChanges.)
+//
+// TODO(quiche): add integration tests for success cases, and update
+// above comment with references to them.
+
+TEST_F(WifiCredentialSyncableServiceTest, AddToSyncedNetworksNotStarted) {
+  EXPECT_FALSE(
+      AddToSyncedNetworks(
+          "fake-item-id",
+          MakeCredential("fake-ssid", SECURITY_CLASS_NONE, "")));
+}
+
+TEST_F(WifiCredentialSyncableServiceTest, AddToSyncedNetworksSuccess) {
+  StartSyncing();
+  EXPECT_TRUE(
+      AddToSyncedNetworks(
+          "fake-item-id",
+          MakeCredential("fake-ssid", SECURITY_CLASS_NONE, "")));
+  EXPECT_EQ(1, GetChangeProcessorChangeCount());
+}
+
+}  // namespace wifi_sync