[sensors] Add Generic Sensor platform unit tests.

device/generic_sensor/platform_sensor_provider_unittest.cc

Index: device/generic_sensor/platform_sensor_provider_unittest.cc
diff --git a/device/generic_sensor/platform_sensor_provider_unittest.cc b/device/generic_sensor/platform_sensor_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b4a66d32bdb9f5af3805743e0e16444a9618f0ba
--- /dev/null
+++ b/device/generic_sensor/platform_sensor_provider_unittest.cc
@@ -0,0 +1,304 @@
+// Copyright 2016 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 "device/generic_sensor/mock_platform_sensor.h"
+#include "device/generic_sensor/mock_platform_sensor_provider.h"
+#include "device/generic_sensor/public/interfaces/sensor_provider.mojom.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace device {
+
+using mojom::SensorReadBuffer;
+using mojom::SensorType;
+
+namespace {
+
+uint64_t GetBufferOffset(mojom::SensorType type) {
+  return (static_cast<uint64_t>(SensorType::LAST) -
+          static_cast<uint64_t>(type)) *
+         SensorReadBuffer::SensorReadBuffer::kReadBufferSize;
+}
+
+}  // namespace
+
+class PlatformSensorTestClient : public PlatformSensor::Client {
+ public:
+  PlatformSensorTestClient()
+      : notification_suspended_(false),
+        sensor_reading_changed_(false),
+        sensor_error_(false) {}
+
+  ~PlatformSensorTestClient() override {
+    if (sensor_)
+      sensor_->RemoveClient(this);
+  }
+
+  // PlatformSensor::Client override.
+  void OnSensorReadingChanged() override { sensor_reading_changed_ = true; }
+
+  void OnSensorError() override { sensor_error_ = true; }
+
+  bool IsNotificationSuspended() override { return notification_suspended_; }
+
+  void set_notification_suspended(bool value) {
+    notification_suspended_ = value;
+  }
+
+  void set_sensor(scoped_refptr<PlatformSensor> sensor) {

[Mikhail, 2016/09/13 10:31:17]

SetSensor

[maksims (do not use this acc), 2016/09/16 09:22:19]

Done.

+    sensor_ = sensor;
+    sensor_->AddClient(this);
+  }
+
+  bool is_sensor_reading_changed() const { return sensor_reading_changed_; }

[timvolodine, 2016/09/13 19:38:53]

sensor_reading_changed()
i.e. trivial getter

[maksims (do not use this acc), 2016/09/16 09:22:19]

Done.

+
+  bool sensor_has_error() const { return sensor_error_; }

[timvolodine, 2016/09/13 19:38:53]

just sensor_error() ?

[maksims (do not use this acc), 2016/09/16 09:22:20]

Well, if I name this method sensor_error(), I would need to return actual error
instead. But this one just says whether there has been an error or not.

[timvolodine, 2016/09/19 17:14:29]

you are returning sensor_error_ which is a boolean already (which makes it a
trivial getter)

[maksims (do not use this acc), 2016/09/21 11:05:19]

Done.

+
+ private:
+  scoped_refptr<PlatformSensor> sensor_;

[timvolodine, 2016/09/13 19:38:53]

is there a reason this cannot be a unique_ptr?

[maksims (do not use this acc), 2016/09/16 09:22:19]

It's implementation specific. PlatformSensor returns scoped_refptr's because of
multithreading issues. For more details, please ask Mikhail or Alexander.

+  bool notification_suspended_;
+  bool sensor_reading_changed_;
+  bool sensor_error_;
+};
+
+class PlatformSensorProviderTest : public ::testing::Test {
+ public:
+  PlatformSensorProviderTest()
+      : sensor_client_(new PlatformSensorTestClient()) {}
+
+ protected:
+  scoped_refptr<PlatformSensor> CreateSensor(mojom::SensorType type) {
+    return MockPlatformSensorProvider::GetInstance()->CreateSensor(
+        type, SensorReadBuffer::kReadBufferSize, GetBufferOffset(type));
+  }
+
+  std::unique_ptr<PlatformSensorTestClient> sensor_client_;
+};
+
+TEST_F(PlatformSensorProviderTest, CreateSensorsAndCheckType) {
+  scoped_refptr<PlatformSensor> sensor1 =

[timvolodine, 2016/09/13 19:38:53]

same here, why not use unique_ptr? and also everywhere below

[maksims (do not use this acc), 2016/09/16 09:22:20]

answered above.

+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  EXPECT_TRUE(sensor1);
+  EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType());
+
+  scoped_refptr<PlatformSensor> sensor2 = CreateSensor(SensorType::PROXIMITY);
+  EXPECT_TRUE(sensor2);
+  EXPECT_EQ(SensorType::PROXIMITY, sensor2->GetType());
+
+  scoped_refptr<PlatformSensor> sensor3 =
+      CreateSensor(SensorType::ACCELEROMETER);
+  EXPECT_TRUE(sensor3);
+  EXPECT_EQ(SensorType::ACCELEROMETER, sensor3->GetType());
+
+  scoped_refptr<PlatformSensor> sensor4 = CreateSensor(SensorType::GYROSCOPE);
+  EXPECT_TRUE(sensor4);
+  EXPECT_EQ(SensorType::GYROSCOPE, sensor4->GetType());
+
+  scoped_refptr<PlatformSensor> sensor5 = CreateSensor(SensorType::PRESSURE);
+  EXPECT_TRUE(sensor5);
+  EXPECT_EQ(SensorType::PRESSURE, sensor5->GetType());
+}
+
+TEST_F(PlatformSensorProviderTest, CreateAndGetSensor) {
+  PlatformSensorProvider* sensor_provider =
+      MockPlatformSensorProvider::GetInstance();
+
+  // Create Ambient Light sensor.
+  scoped_refptr<PlatformSensor> sensor1 =
+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  EXPECT_TRUE(sensor1);
+  EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType());

[timvolodine, 2016/09/13 19:38:53]

can we also have a test for leaks? something like:

scoped_refptr<PlatformSensor> sensor1 =
    CreateSensor(SensorType::AMBIENT_LIGHT);
EXPECT_TRUE(sensor1);
EXPECT_EQ(SensorType::AMBIENT_LIGHT, sensor1->GetType());

sensor1 = nullptr;
EXPECT_EQ(SensorType::AMBIENT_LIGHT,
sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT)->GetType());

[maksims (do not use this acc), 2016/09/16 09:22:19]

No, sensor1 is scoped_refptr. If it is reassigned to nullptr, the test just
crashes.

[timvolodine, 2016/09/19 17:14:29]

why? should it crash? it's refptr so I would expect I can nullify it and then
get back again via GetSensor()?

The reason I actually asked for this test is because I was worried there may be
a leak in:
https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor_pr...
because a raw pointer is stored in map, so just making sure..

[maksims (do not use this acc), 2016/09/21 11:05:19]

Please check now. I've added this case here.

+
+  // Try to get Gyroscope sensor, which has not been created yet.
+  scoped_refptr<PlatformSensor> sensor2 =
+      sensor_provider->GetSensor(SensorType::GYROSCOPE);
+  EXPECT_FALSE(sensor2);
+
+  // Get Ambient Light sensor.
+  scoped_refptr<PlatformSensor> sensor3 =
+      sensor_provider->GetSensor(SensorType::AMBIENT_LIGHT);
+  EXPECT_TRUE(sensor3);
+
+  EXPECT_EQ(sensor1->GetType(), sensor3->GetType());
+
+  // Try to create a sensor with zero buffer and offset.
+  scoped_refptr<PlatformSensor> sensor4 =
+      sensor_provider->CreateSensor(SensorType::GYROSCOPE, 0, 0);
+  EXPECT_FALSE(sensor4);
+
+  scoped_refptr<PlatformSensor> sensor5 =
+      sensor_provider->GetSensor(SensorType::GYROSCOPE);
+  EXPECT_FALSE(sensor5);
+}
+
+// This test assumes that a mock sensor has a constant maximum frequency value
+// of 50 hz (different from the base sensor class that has a range from 0 to
+// 60) and tests whether a mock sensor can be started with a value range from 0
+// to 60.
+TEST_F(PlatformSensorProviderTest, StartListeningWithDifferentParameters) {
+  const double too_high_frequency = 60;
+  const double normal_frequency = 39;
+  scoped_refptr<PlatformSensor> sensor =
+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  MockPlatformSensor* mock_sensor =
+      static_cast<MockPlatformSensor*>(sensor.get());
+  EXPECT_TRUE(mock_sensor);
+  sensor_client_->set_sensor(sensor);
+
+  PlatformSensorConfiguration config(too_high_frequency);
+  EXPECT_EQ(too_high_frequency, config.frequency());
+  EXPECT_FALSE(mock_sensor->StartListening(sensor_client_.get(), config));
+  EXPECT_FALSE(mock_sensor->started());
+
+  config.set_frequency(normal_frequency);
+  EXPECT_EQ(normal_frequency, config.frequency());
+  EXPECT_TRUE(mock_sensor->StartListening(sensor_client_.get(), config));
+  EXPECT_TRUE(mock_sensor->started());
+
+  EXPECT_TRUE(mock_sensor->StopListening(sensor_client_.get(), config));
+  EXPECT_FALSE(mock_sensor->started());
+}
+
+// If a client is in a suspended mode, a NotifySensorReadingChanged()
+// notification must not be sent to the client but NotifySensorError() must be.
+TEST_F(PlatformSensorProviderTest, TestNotificationSuspended) {
+  const int num = 5;
+  scoped_refptr<PlatformSensor> sensor = CreateSensor(SensorType::GYROSCOPE);
+  MockPlatformSensor* mock_sensor =
+      static_cast<MockPlatformSensor*>(sensor.get());
+
+  std::vector<std::unique_ptr<PlatformSensorTestClient>> clients;
+  for (int i = 0; i < num; i++) {
+    std::unique_ptr<PlatformSensorTestClient> client(
+        new PlatformSensorTestClient());
+    client->set_sensor(mock_sensor);
+    clients.push_back(std::move(client));
+  }
+
+  clients.front()->set_notification_suspended(true);
+  mock_sensor->NotifySensorReadingChanged();
+  mock_sensor->NotifySensorError();
+  for (auto const& client : clients) {
+    if (client == clients.front()) {
+      EXPECT_FALSE(client->is_sensor_reading_changed());
+      EXPECT_TRUE(client->sensor_has_error());
+      continue;
+    }
+    EXPECT_TRUE(client->is_sensor_reading_changed());
+    EXPECT_TRUE(client->sensor_has_error());
+  }
+
+  clients.front()->set_notification_suspended(false);
+  mock_sensor->NotifySensorReadingChanged();
+  mock_sensor->NotifySensorError();
+  for (auto const& client : clients) {
+    EXPECT_TRUE(client->is_sensor_reading_changed());
+    EXPECT_TRUE(client->sensor_has_error());
+  }
+}
+
+// Tests that when all clients are removed, config maps are removed as well.
+TEST_F(PlatformSensorProviderTest, TestAddRemoveClients) {
+  const int num = 5;
+  const double frq = 30;
+
+  scoped_refptr<PlatformSensor> sensor =
+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  MockPlatformSensor* mock_sensor =
+      static_cast<MockPlatformSensor*>(sensor.get());
+  EXPECT_TRUE(mock_sensor->config_map().empty());
+
+  std::vector<std::unique_ptr<PlatformSensorTestClient>> clients;
+  PlatformSensorConfiguration config(frq);
+  for (int i = 0; i < num; i++) {
+    std::unique_ptr<PlatformSensorTestClient> client(
+        new PlatformSensorTestClient());
+    client->set_sensor(mock_sensor);
+    EXPECT_TRUE(mock_sensor->StartListening(client.get(), config));
+    EXPECT_TRUE(mock_sensor->started());
+
+    clients.push_back(std::move(client));
+  }
+  EXPECT_FALSE(mock_sensor->config_map().empty());
+
+  for (const auto& client : clients)
+    mock_sensor->RemoveClient(client.get());
+
+  EXPECT_TRUE(mock_sensor->config_map().empty());
+}
+
+// Tests a sensor cannot be updated if it has one suspended client.
+TEST_F(PlatformSensorProviderTest, TestUpdateSensorOneClient) {
+  const double frq = 30;

[timvolodine, 2016/09/13 19:38:53]

any chance of a better name? probably not necessary to have a separate variable
at all if it's used only once below

[timvolodine, 2016/09/19 17:14:29]

?

[maksims (do not use this acc), 2016/09/21 11:05:19]

removed

+
+  scoped_refptr<PlatformSensor> sensor =
+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  MockPlatformSensor* mock_sensor =
+      static_cast<MockPlatformSensor*>(sensor.get());
+  EXPECT_TRUE(mock_sensor->config_map().empty());
+
+  sensor_client_->set_sensor(mock_sensor);
+
+  PlatformSensorConfiguration config(frq);
+  mock_sensor->StartListening(sensor_client_.get(), config);
+
+  sensor_client_->set_notification_suspended(true);
+  EXPECT_TRUE(sensor_client_->IsNotificationSuspended());
+
+  mock_sensor->UpdateSensor();
+  EXPECT_FALSE(mock_sensor->started());
+
+  sensor_client_->set_notification_suspended(false);
+  EXPECT_FALSE(sensor_client_->IsNotificationSuspended());
+
+  mock_sensor->UpdateSensor();
+  EXPECT_TRUE(mock_sensor->started());
+}
+
+// Tests a sensor can be updated if it has one suspended client and other
+// clients are not suspended.
+TEST_F(PlatformSensorProviderTest, TestUpdateSensorManyClients) {
+  const int num = 5;
+
+  scoped_refptr<PlatformSensor> sensor =
+      CreateSensor(SensorType::AMBIENT_LIGHT);
+  MockPlatformSensor* mock_sensor =
+      static_cast<MockPlatformSensor*>(sensor.get());
+  EXPECT_TRUE(mock_sensor->config_map().empty());
+
+  sensor_client_->set_sensor(mock_sensor);
+  std::vector<std::unique_ptr<PlatformSensorTestClient>> clients;
+  for (int i = 0; i < num; i++) {
+    std::unique_ptr<PlatformSensorTestClient> client(
+        new PlatformSensorTestClient());
+    client->set_sensor(mock_sensor);
+    clients.push_back(std::move(client));
+  }
+
+  double frq = 30;

[timvolodine, 2016/09/13 19:38:53]

same here regarding naming

[maksims (do not use this acc), 2016/09/16 09:22:20]

Done.

+  PlatformSensorConfiguration config(frq++);

[timvolodine, 2016/09/13 19:38:53]

why frq++ here?

[maksims (do not use this acc), 2016/09/16 09:22:20]

Nothing special. Just wanted to have different clients with different
frequencies, but it doesn't make any sense to test this here.

+  mock_sensor->StartListening(sensor_client_.get(), config);
+  for (const auto& client : clients) {
+    PlatformSensorConfiguration config(frq++);
+    mock_sensor->StartListening(client.get(), config);
+  }
+
+  sensor_client_->set_notification_suspended(true);
+  EXPECT_TRUE(sensor_client_->IsNotificationSuspended());
+  for (const auto& client : clients)
+    EXPECT_FALSE(client->IsNotificationSuspended());
+
+  mock_sensor->UpdateSensor();
+  EXPECT_TRUE(mock_sensor->started());
+
+  sensor_client_->set_notification_suspended(false);
+  EXPECT_FALSE(sensor_client_->IsNotificationSuspended());
+  for (const auto& client : clients)
+    EXPECT_FALSE(client->IsNotificationSuspended());
+
+  mock_sensor->UpdateSensor();
+  EXPECT_TRUE(mock_sensor->started());
+}
+
+}  // namespace device