[Night Light] CL12: String changes and fix frequent requests.

chrome/browser/chromeos/night_light/night_light_client_unittest.cc

Index: chrome/browser/chromeos/night_light/night_light_client_unittest.cc
diff --git a/chrome/browser/chromeos/night_light/night_light_client_unittest.cc b/chrome/browser/chromeos/night_light/night_light_client_unittest.cc
index bfe124b406301122839a8c8544133a5eaf3b8951..f12f375c764e3cea6159c1e853a4f1af77a9f59c 100644
--- a/chrome/browser/chromeos/night_light/night_light_client_unittest.cc
+++ b/chrome/browser/chromeos/night_light/night_light_client_unittest.cc
@@ -19,6 +19,8 @@ class FakeNightLightController : public ash::mojom::NightLightController {
   FakeNightLightController() : binding_(this) {}
   ~FakeNightLightController() override = default;
 
+  const ash::mojom::SimpleGeopositionPtr& position() const { return position_; }
+
   int position_pushes_num() const { return position_pushes_num_; }
 
   ash::mojom::NightLightControllerPtr CreateInterfacePtrAndBind() {
@@ -65,16 +67,22 @@ class FakeNightLightClient : public NightLightClient {
     position_to_send_ = position;
   }
 
+  int geoposition_requests_num() const { return geoposition_requests_num_; }
+
  private:
   // night_light::NightLightClient:
   void RequestGeoposition() override {
     OnGeoposition(position_to_send_, false, base::TimeDelta());
+    ++geoposition_requests_num_;
   }
 
   // The position to send to the controller the next time OnGeoposition is
   // invoked.
   chromeos::Geoposition position_to_send_;
 
+  // The number of new geoposition requests that have been triggered.
+  int geoposition_requests_num_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(FakeNightLightClient);
 };
 
@@ -119,29 +127,75 @@ TEST_F(NightLightClientTest, TestClientRunningOnlyWhenSunsetToSunriseSchedule) {
 }
 
 // Test that client only pushes valid positions.
-TEST_F(NightLightClientTest, TestPositionPushes) {
-  // Start with a valid position, and expect it to be delivered to the
-  // controller.
+TEST_F(NightLightClientTest, TestInavlidPositions) {

[James Cook, 2017/06/30 22:50:06]

nit: invalid

[afakhry, 2017/06/30 23:41:45]

Done.

   EXPECT_EQ(0, controller_.position_pushes_num());
   chromeos::Geoposition position;
   position.latitude = 32.0;
   position.longitude = 31.0;
-  position.status = chromeos::Geoposition::STATUS_OK;
+  position.status = chromeos::Geoposition::STATUS_TIMEOUT;
   position.accuracy = 10;
   position.timestamp = base::Time::Now();
   client_.set_position_to_send(position);
   controller_.NotifyScheduleTypeChanged(ScheduleType::kSunsetToSunrise);
   scoped_task_environment_.RunUntilIdle();
   client_.FlushNightLightControllerForTesting();
-  EXPECT_EQ(1, controller_.position_pushes_num());
+  EXPECT_EQ(1, client_.geoposition_requests_num());
+  EXPECT_EQ(0, controller_.position_pushes_num());
+}
 
-  // Invalid positions should not be sent.
-  position.status = chromeos::Geoposition::STATUS_TIMEOUT;
-  client_.set_position_to_send(position);
+// Test that successive changes of the schedule type to sunset to sunrise do not
+// trigger repeated geoposition requests.
+TEST_F(NightLightClientTest, TestRepeatedScheduleTypeChanges) {
+  // Start with a valid position, and expect it to be delivered to the
+  // controller.
+  EXPECT_EQ(0, controller_.position_pushes_num());
+  chromeos::Geoposition position1;
+  position1.latitude = 32.0;
+  position1.longitude = 31.0;
+  position1.status = chromeos::Geoposition::STATUS_OK;
+  position1.accuracy = 10;
+  position1.timestamp = base::Time::Now();
+  client_.set_position_to_send(position1);
   controller_.NotifyScheduleTypeChanged(ScheduleType::kSunsetToSunrise);
   scoped_task_environment_.RunUntilIdle();
   client_.FlushNightLightControllerForTesting();
+  EXPECT_EQ(1, client_.geoposition_requests_num());
   EXPECT_EQ(1, controller_.position_pushes_num());
+
+  // A new different position just for the sake of comparison with position1 to
+  // make sure that no new requests are triggered and the same old position will
+  // be resent to the controller.
+  chromeos::Geoposition position2;
+  position2.latitude = 100.0;
+  position2.longitude = 200.0;
+  position2.status = chromeos::Geoposition::STATUS_OK;
+  position2.accuracy = 10;
+  position2.timestamp = base::Time::Now();
+  client_.set_position_to_send(position2);
+  controller_.NotifyScheduleTypeChanged(ScheduleType::kSunsetToSunrise);
+  scoped_task_environment_.RunUntilIdle();
+  client_.FlushNightLightControllerForTesting();
+  // No new request has been triggered, however the same old valid position was
+  // pushed to the controller.
+  EXPECT_EQ(1, client_.geoposition_requests_num());
+  EXPECT_EQ(2, controller_.position_pushes_num());
+  EXPECT_TRUE(ash::mojom::SimpleGeoposition::New(position1.latitude,
+                                                 position1.longitude)
+                  .Equals(controller_.position()));
+
+  // The timer should be running scheduling a next request that is a
+  // kNextRequestDelayAfterSuccess from the last successful request time.
+  EXPECT_TRUE(client_.timer().IsRunning());
+  base::TimeDelta expected_delay =
+      client_.last_successful_geo_request_time() +
+      NightLightClient::GetNextRequestDelayAfterSuccessForTesting() -
+      base::Time::Now();
+  // To avoid flakiness, we get the absolute value of difference between the

[James Cook, 2017/06/30 22:50:06]

This makes me a little nervous. As you mention, testing time is hard. I suggest
either:

* Injecting a Clock and a TickClock into the NightLightClient constructor and
using it to build the timer, which will allow you to use test clocks to exactly
control time, or 
* Putting this bit in a separate test so if it flakes it can be disabled
independently from the rest of the code, or
* Just deleting the part of the test after line 188

[afakhry, 2017/06/30 23:41:46]

I opted for the first suggestion. Please take a look.

+  // expected delay and the actual delay of the timer, and make sure it's less
+  // than an arbitrary epsilon TimeDelta value; say 1 minute.
+  base::TimeDelta delay_delta =
+      (expected_delay - client_.timer().GetCurrentDelay()).magnitude();
+  EXPECT_LT(delay_delta, base::TimeDelta::FromMinutes(1));
 }
 
 }  // namespace