Refactor ResizingHostObserver tests

remoting/host/resizing_host_observer_unittest.cc

Index: remoting/host/resizing_host_observer_unittest.cc
diff --git a/remoting/host/resizing_host_observer_unittest.cc b/remoting/host/resizing_host_observer_unittest.cc
index 08713f1b4aa33072b89331f9fa4a41c80e59d81c..f4aeafde290dd48822ade14fb36beda2526d4f93 100644
--- a/remoting/host/resizing_host_observer_unittest.cc
+++ b/remoting/host/resizing_host_observer_unittest.cc
@@ -40,64 +40,60 @@ ScreenResolution MakeResolution(int width, int height) {
 
 class FakeDesktopResizer : public DesktopResizer {
  public:
-  FakeDesktopResizer(const ScreenResolution& initial_resolution,
-                     bool exact_size_supported,
-                     const ScreenResolution* supported_resolutions,
-                     int num_supported_resolutions,
-                     int* restore_resolution_call_count)
-      : initial_resolution_(initial_resolution),
-        current_resolution_(initial_resolution),
-        exact_size_supported_(exact_size_supported),
-        set_resolution_call_count_(0),
-        restore_resolution_call_count_(restore_resolution_call_count) {
-    for (int i = 0; i < num_supported_resolutions; ++i) {
-      supported_resolutions_.push_back(supported_resolutions[i]);
-    }
+  struct CallCounts {
+    int set_resolution = 0;
+    int restore_resolution = 0;
+  };
+
+  FakeDesktopResizer(bool exact_size_supported,
+                     std::vector<ScreenResolution> supported_resolutions,
+                     ScreenResolution* current_resolution,
+                     CallCounts* call_counts)
+      : exact_size_supported_(exact_size_supported),
+        initial_resolution_(*current_resolution),
+        current_resolution_(current_resolution),
+        supported_resolutions_(std::move(supported_resolutions)),
+        call_counts_(call_counts) {
   }
 
   ~FakeDesktopResizer() override {
     EXPECT_EQ(initial_resolution_, GetCurrentResolution());
   }
 
-  int set_resolution_call_count() { return set_resolution_call_count_; }
-
   // remoting::DesktopResizer interface
   ScreenResolution GetCurrentResolution() override {
-    return current_resolution_;
+    return *current_resolution_;
   }
   std::list<ScreenResolution> GetSupportedResolutions(
       const ScreenResolution& preferred) override {
-    std::list<ScreenResolution> result = supported_resolutions_;
+    std::list<ScreenResolution> result(supported_resolutions_.begin(),
+                                       supported_resolutions_.end());
     if (exact_size_supported_) {
       result.push_back(preferred);
     }
     return result;
   }
   void SetResolution(const ScreenResolution& resolution) override {
-    current_resolution_ = resolution;
-    ++set_resolution_call_count_;
+    *current_resolution_ = resolution;
+    ++call_counts_->set_resolution;
   }
   void RestoreResolution(const ScreenResolution& resolution) override {
-    current_resolution_ = resolution;
-    if (restore_resolution_call_count_)
-      ++(*restore_resolution_call_count_);
+    *current_resolution_ = resolution;
+    ++call_counts_->restore_resolution;
   }
 
  private:
-  ScreenResolution initial_resolution_;
-  ScreenResolution current_resolution_;
   bool exact_size_supported_;
-  std::list<ScreenResolution> supported_resolutions_;
-
-  int set_resolution_call_count_;
-  int* restore_resolution_call_count_;
+  ScreenResolution initial_resolution_;
+  ScreenResolution *current_resolution_;
+  std::vector<ScreenResolution> supported_resolutions_;
+  CallCounts* call_counts_;
 };
 
 class ResizingHostObserverTest : public testing::Test {
  public:
   ResizingHostObserverTest()
-      : desktop_resizer_(nullptr),
-        now_(base::Time::Now()) {
+      : now_(base::Time::Now()) {
   }
 
   // This needs to be public because the derived test-case class needs to
@@ -107,12 +103,16 @@ class ResizingHostObserverTest : public testing::Test {
   }
 
  protected:
-  void SetDesktopResizer(std::unique_ptr<FakeDesktopResizer> desktop_resizer) {
-    CHECK(!desktop_resizer_) << "Call SetDeskopResizer once per test";
-    desktop_resizer_ = desktop_resizer.get();
-
-    resizing_host_observer_.reset(
-        new ResizingHostObserver(std::move(desktop_resizer)));
+  void SetDesktopResizer(const ScreenResolution& initial_resolution,

[Jamie, 2016/06/03 23:47:20]

Maybe rename this InitDesktopResizer, since it's now doing more than just set a
field.

[rkjnsn, 2016/06/04 00:08:21]

Acknowledged.

+                         bool exact_size_supported,
+                         std::vector<ScreenResolution> supported_resolutions) {
+    current_resolution_ = initial_resolution;
+    call_counts_ = FakeDesktopResizer::CallCounts();
+    resizing_host_observer_ = base::MakeUnique<ResizingHostObserver>(
+        base::MakeUnique<FakeDesktopResizer>(exact_size_supported,
+                                             std::move(supported_resolutions),
+                                             &current_resolution_,
+                                             &call_counts_));
     resizing_host_observer_->SetNowFunctionForTesting(
         base::Bind(&ResizingHostObserverTest::GetTimeAndIncrement,
                    base::Unretained(this)));
@@ -120,16 +120,16 @@ class ResizingHostObserverTest : public testing::Test {
 
   ScreenResolution GetBestResolution(const ScreenResolution& client_size) {
     resizing_host_observer_->SetScreenResolution(client_size);
-    return desktop_resizer_->GetCurrentResolution();
+    return current_resolution_;
   }
 
-  void VerifySizes(const ScreenResolution* client_sizes,
-                   const ScreenResolution* expected_sizes,
-                   int number_of_sizes) {
-    for (int i = 0; i < number_of_sizes; ++i) {
-      ScreenResolution best_size = GetBestResolution(client_sizes[i]);
-      EXPECT_EQ(expected_sizes[i], best_size)
-          << "Input resolution = " << client_sizes[i];
+  void VerifySizes(const std::vector<ScreenResolution>& client_sizes,
+                   const std::vector<ScreenResolution>& expected_sizes) {
+    for (auto client = client_sizes.begin(), expected = expected_sizes.begin();

[Jamie, 2016/06/03 23:47:20]

Since we can validate that they are the same size now, we should.

[rkjnsn, 2016/06/04 00:08:21]

Would CHECK_EQ be the preferred way of doing so? Would you leave the for loop
the same, or remove one of the termination conditions?

[Jamie, 2016/06/04 00:29:48]

I don't think it matters tbh. The extra check is just to make sure tests don't
accidentally pass if you have a list size mismatch.

[Jamie, 2016/06/04 00:32:05]

Sorry, I missed the first half of your question. It's been a while since I wrote
C++ unit tests; is CHECK_EQ more like an assert than EXPECT_EQ? If so, then it
seems appropriate, since both values are supplied by test code, so if they don't
match it's not a failure of the code under test.

[rkjnsn, 2016/06/04 00:43:53]

Honestly, I have no idea how the various macros differ. The previous code had
`CHECK(!desktop_resizer_) << "Call SetDeskopResizer once per test";`, which is
similarly verifying the behavior of the test code itself, so I figured this
might call for something similar.

[Jamie, 2016/06/06 19:12:42]

Basically, CHECK does the same thing it does in production code--crashes the
process if it fails. You should only use it if a failure indicates something
wrong with the test code itself (because even a catastrophic failure in the code
under test should be indicated by a test failure, not a test crash). The
difference between ASSERT and TEST is whether or not the test code continues to
run in case of failure; TEST will continue, ASSERT will abort.

+         client != client_sizes.end() && expected != expected_sizes.end();
+         ++client, ++expected) {
+      ScreenResolution best_size = GetBestResolution(*client);
+      EXPECT_EQ(*expected, best_size) << "Input resolution = " << *client;
     }
   }
 
@@ -139,141 +139,112 @@ class ResizingHostObserverTest : public testing::Test {
     return result;
   }
 
-  std::unique_ptr<ResizingHostObserver> resizing_host_observer_;
-  FakeDesktopResizer* desktop_resizer_;
+  ScreenResolution current_resolution_;
+  FakeDesktopResizer::CallCounts call_counts_;
+  std::unique_ptr<ResizingHostObserver> resizing_host_observer_ = nullptr;

[rkjnsn, 2016/06/03 21:33:37]

` = nullptr` isn't actually necessary, because that's what unique_ptr will do by
default. I put it here because the previous version was explicitly initializing
this field to `nullptr`. I'm not sure if it improves clarity.

[Jamie, 2016/06/03 23:47:20]

I think it's fine to remove this. The original code predates unique_ptr, and
perhaps the predecessor didn't have a default ctor.

[rkjnsn, 2016/06/04 00:08:21]

Acknowledged.

   base::Time now_;
 };
 
 // Check that the resolution isn't restored if it wasn't changed by this class.
 TEST_F(ResizingHostObserverTest, NoRestoreResolution) {
-  int restore_resolution_call_count = 0;
-  ScreenResolution initial = MakeResolution(640, 480);
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(new FakeDesktopResizer(
-      initial, false, nullptr, 0, &restore_resolution_call_count));
-  SetDesktopResizer(std::move(desktop_resizer));
-  VerifySizes(nullptr, nullptr, 0);
+  SetDesktopResizer(MakeResolution(640, 480), false, {});
+  VerifySizes({}, {});

[Jamie, 2016/06/03 23:47:20]

I don't think this is needed (and wasn't needed before your change). Passing a
compile-time constant for client_sizes is not actually testing anything other
than the VerifySizes function.

[rkjnsn, 2016/06/04 00:08:21]

Acknowledged.

   resizing_host_observer_.reset();
-  EXPECT_EQ(0, restore_resolution_call_count);
+  EXPECT_EQ(0, call_counts_.restore_resolution);
 }
 
 // Check that the host is not resized if GetSupportedSizes returns an empty
 // list (even if GetCurrentSize is supported).
 TEST_F(ResizingHostObserverTest, EmptyGetSupportedSizes) {
-  int restore_resolution_call_count = 0;
   ScreenResolution initial = MakeResolution(640, 480);
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(new FakeDesktopResizer(
-      initial, false, nullptr, 0, &restore_resolution_call_count));
-  SetDesktopResizer(std::move(desktop_resizer));
-
-  ScreenResolution client_sizes[] = { MakeResolution(200, 100),
-                                      MakeResolution(100, 200) };
-  ScreenResolution expected_sizes[] = { initial, initial };
-  VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
-
+  SetDesktopResizer(initial, false, {});
+  VerifySizes({ MakeResolution(200, 100), MakeResolution(100, 200) },
+              { initial, initial });
   resizing_host_observer_.reset();
-  EXPECT_EQ(0, restore_resolution_call_count);
+  EXPECT_EQ(0, call_counts_.set_resolution);
+  EXPECT_EQ(0, call_counts_.restore_resolution);
 }
 
 // Check that if the implementation supports exact size matching, it is used.
 TEST_F(ResizingHostObserverTest, SelectExactSize) {
-  int restore_resolution_call_count = 0;
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(
-      new FakeDesktopResizer(MakeResolution(640, 480), true, nullptr, 0,
-                             &restore_resolution_call_count));
-  SetDesktopResizer(std::move(desktop_resizer));
-
-  ScreenResolution client_sizes[] = { MakeResolution(200, 100),
-                                      MakeResolution(100, 200),
-                                      MakeResolution(640, 480),
-                                      MakeResolution(480, 640),
-                                      MakeResolution(1280, 1024) };
-  VerifySizes(client_sizes, client_sizes, arraysize(client_sizes));
+  SetDesktopResizer(MakeResolution(640, 480), true, {});
+  std::vector<ScreenResolution> client_sizes { MakeResolution(200, 100),
+                                               MakeResolution(100, 200),
+                                               MakeResolution(640, 480),
+                                               MakeResolution(480, 640),
+                                               MakeResolution(1280, 1024) };
+  VerifySizes(client_sizes, client_sizes);
   resizing_host_observer_.reset();
-  EXPECT_EQ(1, restore_resolution_call_count);
+  EXPECT_EQ(1, call_counts_.restore_resolution);
 }
 
 // Check that if the implementation supports a size that is no larger than
 // the requested size, then the largest such size is used.
 TEST_F(ResizingHostObserverTest, SelectBestSmallerSize) {
-  ScreenResolution supported_sizes[] = { MakeResolution(639, 479),
-                                         MakeResolution(640, 480) };
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(
-      new FakeDesktopResizer(MakeResolution(640, 480), false, supported_sizes,
-                             arraysize(supported_sizes), nullptr));
-  SetDesktopResizer(std::move(desktop_resizer));
-
-  ScreenResolution client_sizes[] = { MakeResolution(639, 479),
-                                      MakeResolution(640, 480),
-                                      MakeResolution(641, 481),
-                                      MakeResolution(999, 999) };
-  ScreenResolution expected_sizes[] = { supported_sizes[0], supported_sizes[1],
-                               supported_sizes[1], supported_sizes[1] };
-  VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
+  std::vector<ScreenResolution> supported_sizes { MakeResolution(639, 479),
+                                                  MakeResolution(640, 480) };
+  SetDesktopResizer(MakeResolution(640, 480), false, supported_sizes);
+  VerifySizes({ MakeResolution(639, 479),
+                MakeResolution(640, 480),
+                MakeResolution(641, 481),
+                MakeResolution(999, 999) },
+              { supported_sizes[0],
+                supported_sizes[1],
+                supported_sizes[1],
+                supported_sizes[1] });
 }
 
 // Check that if the implementation supports only sizes that are larger than
 // the requested size, then the one that requires the least down-scaling.
 TEST_F(ResizingHostObserverTest, SelectBestScaleFactor) {
-  ScreenResolution supported_sizes[] = { MakeResolution(100, 100),
-                                         MakeResolution(200, 100) };
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(
-      new FakeDesktopResizer(MakeResolution(200, 100), false, supported_sizes,
-                             arraysize(supported_sizes), nullptr));
-  SetDesktopResizer(std::move(desktop_resizer));
-
-  ScreenResolution client_sizes[] = { MakeResolution(1, 1),
-                                      MakeResolution(99, 99),
-                                      MakeResolution(199, 99) };
-  ScreenResolution expected_sizes[] = { supported_sizes[0], supported_sizes[0],
-                               supported_sizes[1] };
-  VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
+  std::vector<ScreenResolution> supported_sizes { MakeResolution(100, 100),
+                                                  MakeResolution(200, 100) };
+  SetDesktopResizer(MakeResolution(200, 100), false, supported_sizes);
+  VerifySizes({ MakeResolution(1, 1),
+                MakeResolution(99, 99),
+                MakeResolution(199, 99) },
+              { supported_sizes[0],
+                supported_sizes[0],
+                supported_sizes[1] });
 }
 
 // Check that if the implementation supports two sizes that have the same
 // resultant scale factor, then the widest one is selected.
 TEST_F(ResizingHostObserverTest, SelectWidest) {
-  ScreenResolution supported_sizes[] = { MakeResolution(640, 480),
-                                         MakeResolution(480, 640) };
-  std::unique_ptr<FakeDesktopResizer> desktop_resizer(
-      new FakeDesktopResizer(MakeResolution(480, 640), false, supported_sizes,
-                             arraysize(supported_sizes), nullptr));
-  SetDesktopResizer(std::move(desktop_resizer));
-
-  ScreenResolution client_sizes[] = { MakeResolution(100, 100),
-                                      MakeResolution(480, 480),
-                                      MakeResolution(500, 500),
-                                      MakeResolution(640, 640),
-                                      MakeResolution(1000, 1000) };
-  ScreenResolution expected_sizes[] = { supported_sizes[0], supported_sizes[0],
-                               supported_sizes[0], supported_sizes[0],
-                               supported_sizes[0] };
-  VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
+  std::vector<ScreenResolution> supported_sizes { MakeResolution(640, 480),
+                                                  MakeResolution(480, 640) };
+  SetDesktopResizer(MakeResolution(480, 640), false, supported_sizes);
+  VerifySizes({ MakeResolution(100, 100),
+                MakeResolution(480, 480),
+                MakeResolution(500, 500),
+                MakeResolution(640, 640),
+                MakeResolution(1000, 1000) },
+              { supported_sizes[0],
+                supported_sizes[0],
+                supported_sizes[0],
+                supported_sizes[0],
+                supported_sizes[0] });
 }
 
 // Check that if the best match for the client size doesn't change, then we
 // don't call SetSize.
 TEST_F(ResizingHostObserverTest, NoSetSizeForSameSize) {
-  ScreenResolution supported_sizes[] = { MakeResolution(640, 480),
-                                         MakeResolution(480, 640) };
-  SetDesktopResizer(base::WrapUnique(
-      new FakeDesktopResizer(MakeResolution(480, 640), false, supported_sizes,
-                             arraysize(supported_sizes), nullptr)));
-
-  ScreenResolution client_sizes[] = { MakeResolution(640, 640),
-                                      MakeResolution(1024, 768),
-                                      MakeResolution(640, 480) };
-  ScreenResolution expected_sizes[] = { MakeResolution(640, 480),
-                                        MakeResolution(640, 480),
-                                        MakeResolution(640, 480) };
-  VerifySizes(client_sizes, expected_sizes, arraysize(client_sizes));
-  EXPECT_EQ(desktop_resizer_->set_resolution_call_count(), 1);
+  std::vector<ScreenResolution> supported_sizes { MakeResolution(640, 480),
+                                                  MakeResolution(480, 640) };
+  SetDesktopResizer(MakeResolution(480, 640), false, supported_sizes);
+  VerifySizes({ MakeResolution(640, 640),
+                MakeResolution(1024, 768),
+                MakeResolution(640, 480) },
+              { supported_sizes[0],
+                supported_sizes[0],
+                supported_sizes[0] });
+  EXPECT_EQ(1, call_counts_.set_resolution);
 }
 
 // Check that desktop resizes are rate-limited, and that if multiple resize
 // requests are received in the time-out period, the most recent is respected.
 TEST_F(ResizingHostObserverTest, RateLimited) {
-  SetDesktopResizer(base::WrapUnique(new FakeDesktopResizer(
-      MakeResolution(640, 480), true, nullptr, 0, nullptr)));
+  SetDesktopResizer(MakeResolution(640, 480), true, {});
   resizing_host_observer_->SetNowFunctionForTesting(
       base::Bind(&ResizingHostObserverTest::GetTime, base::Unretained(this)));
 
@@ -302,7 +273,7 @@ TEST_F(ResizingHostObserverTest, RateLimited) {
   run_loop.Run();
 
   // If the QuitClosure fired before the final resize, it's a test failure.
-  EXPECT_EQ(desktop_resizer_->GetCurrentResolution(), MakeResolution(300, 300));
+  EXPECT_EQ(current_resolution_, MakeResolution(300, 300));
 }
 
 }  // namespace remoting