If requested, report network interfaces to management server.

chrome/browser/chromeos/policy/device_status_collector_browsertest.cc

Index: chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
diff --git a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
index 12af15eb319dd02b7364a133fcb78b62307138a0..04f3ad99f931b9fe91d2e608a3181e3b0027bbba 100644
--- a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
@@ -10,6 +10,7 @@
 #include "base/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/testing_pref_service.h"
+#include "base/run_loop.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/cros_settings_names.h"
@@ -20,12 +21,16 @@
 #include "chrome/browser/chromeos/system/statistics_provider.h"
 #include "chrome/browser/policy/proto/cloud/device_management_backend.pb.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_device_client.h"
+#include "chromeos/network/network_handler.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/geolocation_provider.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
 
 using ::testing::DoAll;
 using ::testing::NotNull;
@@ -582,4 +587,86 @@ TEST_F(DeviceStatusCollectorTest, Location) {
   CheckThatALocationErrorIsReported();
 }
 
+// Fake device state.
+struct FakeDeviceData {
+  const char* device_path;
+  const char* type;
+  const char* object_path;
+  const char* mac_address;
+  const char* meid;
+};
+
+static const FakeDeviceData kFakeDevices[] = {
+  { "/device/ethernet", flimflam::kTypeEthernet, "ethernet",
+    "112233445566", NULL },

[pneubeck (no reviews), 2013/07/04 15:51:48]

nit: maybe test also a device without mac_address

[Mattias Nissler (ping if slow), 2013/07/11 17:41:46]

Done.

+  { "/device/cellular", flimflam::kTypeCellular, "cellular",
+    "abcdefabcdef", "meid", },
+  { "/device/wifi", flimflam::kTypeWifi, "wifi",
+    "aabbccddeeff", NULL, },
+};
+
+class DeviceStatusCollectorNetworkInterfacesTest
+    : public DeviceStatusCollectorTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    chromeos::DBusThreadManager::InitializeWithStub();
+    chromeos::NetworkHandler::Initialize();
+    chromeos::ShillDeviceClient::TestInterface* test_device_client =
+        chromeos::DBusThreadManager::Get()->GetShillDeviceClient()->
+            GetTestInterface();
+    test_device_client->ClearDevices();
+    for (size_t i = 0; i < arraysize(kFakeDevices); ++i) {
+      const FakeDeviceData& dev = kFakeDevices[i];
+      test_device_client->AddDevice(dev.device_path, dev.type,
+                                    dev.object_path);
+      if (dev.mac_address) {
+        base::StringValue value(dev.mac_address);

[pneubeck (no reviews), 2013/07/04 15:51:48]

nit: no need for local variable declaration |value|

[Mattias Nissler (ping if slow), 2013/07/11 17:41:46]

Done.

Clang used to complain in these cases. That was because of the copy constructor
required to pass the temporary not existing, which is no longer the case.

+        test_device_client->SetDeviceProperty(
+            dev.device_path, flimflam::kAddressProperty, value);
+      }
+      if (dev.meid) {
+        base::StringValue value(dev.meid);

[pneubeck (no reviews), 2013/07/04 15:51:48]

ditto

[Mattias Nissler (ping if slow), 2013/07/11 17:41:46]

Done.

+        test_device_client->SetDeviceProperty(
+            dev.device_path, flimflam::kMeidProperty, value);
+      }
+    }
+
+    // Flush out pending state updates.
+    base::RunLoop().RunUntilIdle();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    chromeos::NetworkHandler::Shutdown();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+};
+
+TEST_F(DeviceStatusCollectorNetworkInterfacesTest, NetworkInterfaces) {
+  // No interfaces should be reported if the policy is off.
+  GetStatus();
+  EXPECT_EQ(0, status_.network_interface_size());
+
+  // Switch the policy on and verify the interface list is present.
+  cros_settings_->SetBoolean(chromeos::kReportDeviceNetworkInterfaces, true);
+  GetStatus();
+
+  ASSERT_EQ(static_cast<int>(arraysize(kFakeDevices)),
+            status_.network_interface_size());
+  for (size_t i = 0; i < arraysize(kFakeDevices); ++i) {
+    const em::NetworkInterface& iface = status_.network_interface(i);
+    const FakeDeviceData& dev = kFakeDevices[i];
+    EXPECT_EQ(dev.type, iface.type());
+
+    if (dev.mac_address)

[pneubeck (no reviews), 2013/07/04 15:51:48]

you're testing the logic in the implementation with the same logic reimplemented
in the test.

wouldn't it have been an alternative to serialize the protobuf message to a JSON
string and compare that to an expected string?

that could also be used in other tests of this kind.

[Mattias Nissler (ping if slow), 2013/07/11 17:41:46]

That doesn't make a difference, I'd just be swapping one format of specifying
test expectations for another without gaining any test coverage quality.
Ideally, I'd test this against what shill gives me, but that'd require an
integration test with Chrome OS that can't be done as browser_test.

+      EXPECT_EQ(dev.mac_address, iface.mac_address());
+    else
+      EXPECT_FALSE(iface.has_mac_address());
+
+    if (dev.meid)
+      EXPECT_EQ(dev.meid, iface.meid());
+    else
+      EXPECT_FALSE(iface.has_meid());
+  }
+}
+
 }  // namespace policy