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

chrome/browser/chromeos/night_light/night_light_client_unittest.cc

 // Copyright 2017 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 "chrome/browser/chromeos/night_light/night_light_client.h"
 
 #include "ash/public/interfaces/night_light_controller.mojom.h"
 #include "base/test/scoped_task_environment.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 using ScheduleType = ash::mojom::NightLightController::ScheduleType;
 
 // A fake implementation of NightLightController for testing.
 class FakeNightLightController : public ash::mojom::NightLightController {
  public:
   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() {
     ash::mojom::NightLightControllerPtr ptr;
     binding_.Bind(mojo::MakeRequest(&ptr));
     return ptr;
   }
 
   // ash::mojom::NightLightController:
   void SetCurrentGeoposition(
@@ skipping 26 matching lines @@
 // geoposition requests.
 class FakeNightLightClient : public NightLightClient {
  public:
   FakeNightLightClient() : NightLightClient(nullptr /* url_context_getter */) {}
   ~FakeNightLightClient() override = default;
 
   void set_position_to_send(const chromeos::Geoposition& position) {
     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);
 };
 
 // Base test fixture.
 class NightLightClientTest : public testing::Test {
  public:
   NightLightClientTest() = default;
   ~NightLightClientTest() override = default;
 
   void SetUp() override {
@@ skipping 24 matching lines @@
   client_.FlushNightLightControllerForTesting();
   EXPECT_TRUE(client_.using_geoposition());
 
   // Client should stop retrieving geopositions when schedule type changes to
   // something else.
   controller_.NotifyScheduleTypeChanged(ScheduleType::kNone);
   EXPECT_FALSE(client_.using_geoposition());
 }
 
 // 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