Updated DeviceStatusReportRequest to contain new monitoring data.

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 68d70728cfefe679f83f20ed85dc6d618ffd475f..a8fdb8c01f8c3e3646b379aa952d6ea77aa0fb60 100644
--- a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
@@ -23,7 +23,10 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
+#include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
 #include "chromeos/system/fake_statistics_provider.h"
@@ -683,6 +686,46 @@ static const FakeDeviceData kFakeDevices[] = {
     -1 },
 };
 
+// Fake network state.
+struct FakeNetworkState {
+  const char* name;
+  const char* device_path;
+  const char* type;
+  const int signal_strength;

[pneubeck (no reviews), 2014/12/09 03:00:10]

nit: const not required

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Done.

+  const char* connection_status;
+  int expected_state;
+};
+
+// List of fake networks - primarily used to make sure that signal strength
+// and connection state are properly populated in status reports. Note that
+// by convention shill will not report a signal strength of "0" for a visible

[pneubeck (no reviews), 2014/12/09 03:00:10]

nit: "0" -> 0
for consistency.

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Done.

+// network, so we use 1 below.
+static const FakeNetworkState kFakeNetworks[] = {
+  { "offline", "/device/wifi", shill::kTypeWifi, 35,
+    shill::kStateOffline, em::NetworkState::OFFLINE },
+  { "ethernet", "/device/ethernet", shill::kTypeEthernet, 0,
+    shill::kStateOnline, em::NetworkState::ONLINE },
+  { "wifi", "/device/wifi", shill::kTypeWifi, 23, shill::kStatePortal,
+    em::NetworkState::PORTAL },
+  { "idle", "/device/cellular1", shill::kTypeCellular, 0, shill::kStateIdle,
+    em::NetworkState::IDLE },
+  { "carrier", "/device/cellular1", shill::kTypeCellular, 0,
+    shill::kStateCarrier, em::NetworkState::CARRIER },
+  { "association", "/device/cellular1", shill::kTypeCellular, 0,
+    shill::kStateAssociation, em::NetworkState::ASSOCIATION },
+  { "config", "/device/cellular1", shill::kTypeCellular, 0,
+    shill::kStateConfiguration, em::NetworkState::CONFIGURATION },
+  { "ready", "/device/cellular1", shill::kTypeCellular, 0, shill::kStateReady,
+    em::NetworkState::READY },
+  { "disconnect", "/device/wifi", shill::kTypeWifi, 1,
+    shill::kStateDisconnect, em::NetworkState::DISCONNECT },
+  { "failure", "/device/wifi", shill::kTypeWifi, 1, shill::kStateFailure,
+    em::NetworkState::FAILURE },
+  { "activation-failure", "/device/cellular1", shill::kTypeCellular, 0,
+    shill::kStateActivationFailure, em::NetworkState::ACTIVATION_FAILURE },
+  { "unknown", "", shill::kTypeWifi, 1, "unknown", em::NetworkState::UNKNOWN },
+};
+
 class DeviceStatusCollectorNetworkInterfacesTest
     : public DeviceStatusCollectorTest {
  protected:
@@ -714,8 +757,41 @@ class DeviceStatusCollectorNetworkInterfacesTest
       }
     }
 
+    chromeos::ShillServiceClient::TestInterface* service_client =
+        chromeos::DBusThreadManager::Get()->GetShillServiceClient()->
+            GetTestInterface();
+    service_client->ClearServices();
+
+    // Now add services for every fake network.
+    for (size_t i = 0; i < arraysize(kFakeNetworks); ++i) {

[pneubeck (no reviews), 2014/12/09 03:00:10]

should be possible to replace by

for (const FakeNetworkState& fake_network : kFakeNetworks) {
 ...

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Done.

+      service_client->AddService(
+          kFakeNetworks[i].name,       /* service_path */
+          kFakeNetworks[i].name        /* guid */,
+          kFakeNetworks[i].name        /* name */,
+          kFakeNetworks[i].type        /* type */,
+          kFakeNetworks[i].connection_status,
+          true /* visible */);

[pneubeck (no reviews), 2014/12/09 03:00:10]

would you want to test a not visible network as well, since you include them in
the report?

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Added a test for this (we have a disconnected network and I've made it
non-visible).

+      service_client->SetServiceProperty(
+          kFakeNetworks[i].name, shill::kSignalStrengthProperty,
+          base::FundamentalValue(kFakeNetworks[i].signal_strength));
+      service_client->SetServiceProperty(
+          kFakeNetworks[i].name, shill::kDeviceProperty,
+          base::StringValue(kFakeNetworks[i].device_path));
+    }
+
     // Flush out pending state updates.
     base::RunLoop().RunUntilIdle();
+
+    chromeos::NetworkStateHandler::NetworkStateList state_list;
+    chromeos::NetworkStateHandler* network_state_handler =
+        chromeos::NetworkHandler::Get()->network_state_handler();
+    network_state_handler->GetNetworkListByType(
+        chromeos::NetworkTypePattern::Default(),
+        false,  // configured_only
+        false,  // visible_only,
+        0,      // no limit to number of results
+        &state_list);
+    ASSERT_EQ(arraysize(kFakeNetworks), state_list.size());
   }
 
   virtual void TearDown() override {
@@ -728,11 +804,13 @@ TEST_F(DeviceStatusCollectorNetworkInterfacesTest, NetworkInterfaces) {
   // Interfaces should be reported by default.
   GetStatus();
   EXPECT_LT(0, status_.network_interface_size());
+  EXPECT_LT(0, status_.network_state_size());
 
   // No interfaces should be reported if the policy is off.
   cros_settings_->SetBoolean(chromeos::kReportDeviceNetworkInterfaces, false);
   GetStatus();
   EXPECT_EQ(0, status_.network_interface_size());
+  EXPECT_EQ(0, status_.network_state_size());
 
   // Switch the policy on and verify the interface list is present.
   cros_settings_->SetBoolean(chromeos::kReportDeviceNetworkInterfaces, true);
@@ -758,7 +836,8 @@ TEST_F(DeviceStatusCollectorNetworkInterfacesTest, NetworkInterfaces) {
           iface->has_imei() == !!*dev.imei &&
           iface->mac_address() == dev.mac_address &&
           iface->meid() == dev.meid &&
-          iface->imei() == dev.imei) {
+          iface->imei() == dev.imei &&
+          iface->device_path() == dev.device_path) {
         found_match = true;
         break;
       }
@@ -769,6 +848,29 @@ TEST_F(DeviceStatusCollectorNetworkInterfacesTest, NetworkInterfaces) {
   }
 
   EXPECT_EQ(count, status_.network_interface_size());
+
+  // Now make sure network state list is correct.
+  EXPECT_EQ(arraysize(kFakeNetworks),
+            static_cast<size_t>(status_.network_state_size()));
+  for (size_t i = 0; i < arraysize(kFakeNetworks); ++i) {

[pneubeck (no reviews), 2014/12/09 03:00:10]

same here

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Done.

+    const FakeNetworkState& state = kFakeNetworks[i];
+    bool found_match = false;
+    google::protobuf::RepeatedPtrField<em::NetworkState>::const_iterator
+        proto_state;
+    for (proto_state = status_.network_state().begin();

[pneubeck (no reviews), 2014/12/09 03:00:10]

for (const em::NetworkState& proto_state : status_.network_state()) {
 ...

[Andrew T Wilson (Slow), 2014/12/11 00:09:55]

Done.

+         proto_state != status_.network_state().end();
+         ++proto_state) {
+      // Make sure every item has a matching entry in the proto.
+      if (proto_state->has_device_path() == (strlen(state.device_path) > 0) &&
+          proto_state->signal_strength() == state.signal_strength &&
+          proto_state->connection_state() == state.expected_state) {
+        found_match = true;
+        break;
+      }
+    }
+    EXPECT_TRUE(found_match) << "No matching state for fake network " << i
+                             << " (" << state.name << ")";
+  }
 }
 
 }  // namespace policy