wifi_sync: implement ACTION_ADD in WifiCredentialSyncableService

components/wifi_sync/wifi_credential_syncable_service_unittest.cc

+// Copyright 2015 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 "components/wifi_sync/wifi_credential_syncable_service.h"
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
+#include "base/tracked_objects.h"
+#include "components/wifi_sync/wifi_config_delegate.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "components/wifi_sync/wifi_security_class.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/fake_sync_change_processor.h"
+#include "sync/api/sync_change.h"
+#include "sync/api/sync_data.h"
+#include "sync/api/sync_error.h"
+#include "sync/api/sync_error_factory_mock.h"
+#include "sync/internal_api/public/attachments/attachment_service_proxy_for_test.h"
+#include "sync/protocol/sync.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi_sync {
+
+using syncer::FakeSyncChangeProcessor;
+using syncer::SyncErrorFactoryMock;
+
+// Fake implementation of WifiConfigDelegate, which provides the

[erikwright (departed), 2015/01/22 15:59:10]

You can start your anon namespace here.

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+// ability to check how many times a WifiConfigDelegate method has
+// been called.
+class FakeWifiConfigDelegate : public WifiConfigDelegate {
+ public:
+  FakeWifiConfigDelegate() : add_count_(0) {}
+  virtual ~FakeWifiConfigDelegate() {}
+
+  // WifiConfigDelegate implementation.
+  void AddToLocalNetworks(const WifiCredential& network_credential) override {
+    ++add_count_;
+  }
+
+  // Returns the number of times the AddToLocalNetworks method has
+  // been called.
+  int GetAddCount() const { return add_count_; }

[erikwright (departed), 2015/01/22 15:59:10]

accessor naming (add_count())

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+
+ private:
+  // The number of times AddToLocalNetworks has been called on this fake.
+  int add_count_;

[erikwright (departed), 2015/01/22 15:59:09]

unsigned int

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(FakeWifiConfigDelegate);
+};
+
+// Unit tests for WifiCredentialSyncableService.
+class WifiCredentialSyncableServiceTest : public testing::Test {
+ protected:
+  WifiCredentialSyncableServiceTest()
+      : config_delegate_(new FakeWifiConfigDelegate()),
+        change_processor_(nullptr) {
+    syncable_service_.reset(
+        new WifiCredentialSyncableService(make_scoped_ptr(config_delegate_)));

[erikwright (departed), 2015/01/22 15:59:10]

This can appear in the initializer list.

[mukesh agrawal, 2015/01/22 23:36:03]

In order to place this in the initializer list, I'll need to change the
declaration order. Currently, I have:

  scoped_ptr<WifiCredentialSyncableService> syncable_service_;
  FakeWifiConfigDelegate* config_delegate_;    // Owned by |syncable_service_|
  FakeSyncChangeProcessor* change_processor_;  // Owned by |syncable_service_|

With this declaration order, placing syncable_service_ in the initializer list
would mean that I read from config_delegate_ before  config_delegate_ has been
initialized.

I think the current declaration order is preferable, as it introduces
|syncable_service_| before the comments that reference it.

If you disagree, let me know, and I'll change it.

+  }
+
+  // Wrappers for methods in WifiCredentialSyncableService.
+  syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& caller_location,
+      const syncer::SyncChangeList& change_list) {
+    return syncable_service_->ProcessSyncChanges(caller_location, change_list);
+  }
+  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() {

[erikwright (departed), 2015/01/22 15:59:10]

accessor naming

[mukesh agrawal, 2015/01/22 23:36:03]

Done. (I couldn't find any guidance, in the style guide, for naming an accessor
which follows references. So I simply concatenated the names.)

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

[erikwright (departed), 2015/01/22 15:59:10]

ditto

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+
+  // Returns a new WifiCredential constructed from the given parameters.
+  WifiCredential MakeCredential(const std::string& ssid,
+                                WifiSecurityClass security_class,
+                                const std::string& passphrase) {
+    scoped_ptr<WifiCredential> credential = WifiCredential::Create(
+        WifiCredential::MakeSsidBytesForTest(ssid), security_class, passphrase);
+    CHECK(credential);
+    return *credential;
+  }
+
+  // Returns a new EntitySpecifics protobuf, with the
+  // wifi_credential_specifics fields populated with the given
+  // parameters.
+  sync_pb::EntitySpecifics MakeWifiCredentialSpecifics(
+      const std::string& ssid,
+      sync_pb::WifiCredentialSpecifics_SecurityClass security_class,
+      const std::string* passphrase) {
+    const std::vector<uint8_t> ssid_bytes(ssid.begin(), ssid.end());
+    sync_pb::EntitySpecifics sync_entity_specifics;
+    sync_pb::WifiCredentialSpecifics* wifi_credential_specifics =
+        sync_entity_specifics.mutable_wifi_credential();
+    CHECK(wifi_credential_specifics);
+    wifi_credential_specifics->set_ssid(ssid_bytes.data(), ssid_bytes.size());
+    wifi_credential_specifics->set_security_class(security_class);
+    if (passphrase) {
+      wifi_credential_specifics->set_passphrase(passphrase->data(),
+                                                passphrase->size());
+    }
+    return sync_entity_specifics;
+  }
+
+  syncer::SyncChange MakeActionAdd(
+      const tracked_objects::Location& caller_location,
+      int sync_item_id,
+      const std::string& ssid,
+      sync_pb::WifiCredentialSpecifics_SecurityClass security_class,
+      const std::string* passphrase) {
+    return syncer::SyncChange(
+        caller_location, syncer::SyncChange::ACTION_ADD,
+        syncer::SyncData::CreateRemoteData(
+            sync_item_id,
+            MakeWifiCredentialSpecifics(ssid, security_class, passphrase),
+            base::Time(), syncer::AttachmentIdList(),
+            syncer::AttachmentServiceProxyForTest::Create()));
+  }
+
+  void StartSyncing() {
+    scoped_ptr<FakeSyncChangeProcessor> change_processor(
+        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);
+};
+
+namespace {
+const char kSsidNonUtf8[] = "\xc0";
+}

[erikwright (departed), 2015/01/22 15:59:09]

}  // namespace

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+
+TEST_F(WifiCredentialSyncableServiceTest, ProcessSyncChangesNotStarted) {
+  syncer::SyncError sync_error(
+      ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList()));

[erikwright (departed), 2015/01/22 15:59:09]

Unless you actually test something related to the location, why not just pull
the "FROM_HERE" inside the helper method and simplify the signature?

[mukesh agrawal, 2015/01/22 23:36:03]

Done. (Made the same change for MakeActionAdd.)

+  ASSERT_TRUE(sync_error.IsSet());
+  EXPECT_EQ(syncer::SyncError::UNREADY_ERROR, sync_error.error_type());
+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassInvalid) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_INVALID, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());  // Just ignore bad items.

[erikwright (departed), 2015/01/22 15:59:10]

// Bad items are ignored.

The current comment seems to describe what the test does (it confused me). My
suggestion more clearly describes what the code under test does.

Do this below, too.

[mukesh agrawal, 2015/01/22 23:36:03]

Done.

+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassNoneSuccess) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_NONE, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());
+  EXPECT_EQ(1, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassWepMissingPassphrase) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_WEP, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());  // Just ignore bad items.
+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassWepSuccess) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  const std::string passphrase("abcde");
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_WEP, &passphrase));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());
+  EXPECT_EQ(1, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassPskMissingPassphrase) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_PSK, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());  // Just ignore bad items.
+  EXPECT_EQ(0, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesActionAddSecurityClassPskSuccess) {
+  syncer::SyncChangeList changes;
+  const int sync_item_id = 1;
+  const std::string passphrase("psk-passphrase");
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, sync_item_id, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_PSK, &passphrase));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());
+  EXPECT_EQ(1, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesContinuesAfterSecurityClassInvalid) {
+  syncer::SyncChangeList changes;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, 1 /* sync item id */, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_INVALID, nullptr));
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, 2 /* sync item id */, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_NONE, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());  // Just ignore bad items.
+  EXPECT_EQ(1, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest,
+       ProcessSyncChangesContinuesAfterMissingPassphrase) {
+  syncer::SyncChangeList changes;
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, 1 /* sync item id */, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_WEP, nullptr));
+  changes.push_back(MakeActionAdd(
+      FROM_HERE, 2 /* sync item id */, kSsidNonUtf8,
+      sync_pb::WifiCredentialSpecifics::SECURITY_CLASS_NONE, nullptr));
+  StartSyncing();
+  syncer::SyncError sync_error = ProcessSyncChanges(FROM_HERE, changes);
+  EXPECT_FALSE(sync_error.IsSet());  // Just ignore bad items.
+  EXPECT_EQ(1, GetWifiDelegateAddCount());
+}
+
+TEST_F(WifiCredentialSyncableServiceTest, AddToSyncedNetworksNotStarted) {
+  EXPECT_FALSE(AddToSyncedNetworks(
+      "fake-item-id", MakeCredential(kSsidNonUtf8, SECURITY_CLASS_NONE, "")));
+}
+
+TEST_F(WifiCredentialSyncableServiceTest, AddToSyncedNetworksSuccess) {
+  StartSyncing();
+  EXPECT_TRUE(AddToSyncedNetworks(
+      "fake-item-id", MakeCredential(kSsidNonUtf8, SECURITY_CLASS_NONE, "")));
+  EXPECT_EQ(1, GetChangeProcessorChangeCount());
+}
+
+}  // namespace wifi_sync