[SystemInfo API] Finish TODOs in SystemInfoProvider

chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc

 // Copyright 2013 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.
 
 // DisplayInfoProvider unit tests.
 
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/api/system_info_display/display_info_provider.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 22 matching lines @@
 
 const struct TestDisplayInfo kTestingDisplayInfoData[] = {
   {"Display1", "Display Name1", true, true, true, 96.0, 96.0,
       {0, 0, 1280, 720}, {0, 0, 960, 720}},
   {"Display2", "Display Name2", true, true, true, 221.0, 221.0,
       {0, 0, 1280, 720}, {0, 0, 960, 720}}
 };
 
 class TestDisplayInfoProvider : public DisplayInfoProvider {
  public:
-  virtual bool QueryInfo(DisplayInfo* info) OVERRIDE;
+  virtual bool QueryInfo() OVERRIDE;
 
  private:
   virtual ~TestDisplayInfoProvider();
 };
 
 TestDisplayInfoProvider::~TestDisplayInfoProvider() {}
 
-bool TestDisplayInfoProvider::QueryInfo(DisplayInfo* info) {
-  if (info == NULL)
-    return false;
-  info->clear();
+bool TestDisplayInfoProvider::QueryInfo() {
+  info_.clear();
   for (size_t i = 0; i < arraysize(kTestingDisplayInfoData); ++i) {
     linked_ptr<DisplayUnitInfo> unit(new DisplayUnitInfo());
     unit->id = kTestingDisplayInfoData[i].id;
     unit->name = kTestingDisplayInfoData[i].name;
     unit->is_enabled = kTestingDisplayInfoData[i].is_enabled;
     unit->is_primary = kTestingDisplayInfoData[i].is_primary;
     unit->is_internal = kTestingDisplayInfoData[i].is_internal;
     unit->dpi_x = kTestingDisplayInfoData[i].dpi_x;
     unit->dpi_y = kTestingDisplayInfoData[i].dpi_y;
     unit->bounds.left = kTestingDisplayInfoData[i].bounds.left;
     unit->bounds.top = kTestingDisplayInfoData[i].bounds.top;
     unit->bounds.width = kTestingDisplayInfoData[i].bounds.width;
     unit->bounds.height = kTestingDisplayInfoData[i].bounds.height;
     unit->work_area.left = kTestingDisplayInfoData[i].work_area.left;
     unit->work_area.top = kTestingDisplayInfoData[i].work_area.top;
     unit->work_area.width = kTestingDisplayInfoData[i].work_area.width;
     unit->work_area.height = kTestingDisplayInfoData[i].work_area.height;
-    info->push_back(unit);
+    info_.push_back(unit);
   }
   return true;
 }
 
 class DisplayInfoProviderTest : public testing::Test {
  public:
   DisplayInfoProviderTest();
 
  protected:
   scoped_refptr<TestDisplayInfoProvider> display_info_provider_;
 };
 
 DisplayInfoProviderTest::DisplayInfoProviderTest()
     : display_info_provider_(new TestDisplayInfoProvider()) {
 }
 
 TEST_F(DisplayInfoProviderTest, QueryDisplayInfo) {
-  scoped_ptr<DisplayInfo> display_info(new DisplayInfo());
-  EXPECT_TRUE(display_info_provider_->QueryInfo(display_info.get()));
-  EXPECT_EQ(arraysize(kTestingDisplayInfoData), display_info->size());
+  EXPECT_TRUE(display_info_provider_->QueryInfo());
+  const DisplayInfo& display_info = display_info_provider_->display_info();

[Jeffrey Yasskin, 2013/07/08 19:09:49]

FWIW, this doesn't test anything that runs in the production system except the
type definitions of DisplayInfo and api::system_info_display::DisplayUnitInfo.
You've overridden QueryInfo in the test fixture, and that's all you're calling
here, so no production code runs.

You didn't make this worse in this CL, but I wanted to point it out so that Greg
doesn't think these APIs are ready to escape experimental before it's fixed.

[Haojian Wu, 2013/07/09 13:45:04]

Your suggestion reminds me that the unit_tests of CpuInfoProvider,
MemoryInfoProvider, DisplayInfoProvider
are the same, just overriding the QueryInfo. We need to improve the way of
testing SystemInfo API, especially StorageInfo api. 

About the unit_tests here, as you said, Are they unnecessary? Should we remove
them?

[Jeffrey Yasskin, 2013/07/09 18:19:59]

Yeah, we should remove them and replace them (likely in a future CL) with
something that actually tests the production code. It's possible that these
classes are too thin to be worth testing in a unit test, or that it's extremely
hard to mock the underlying system results, in which case you should just use
the apitest to make sure that the functions exist all the way down and return
results that match what more direct calls return.

By the way, this is why I don't like putting virtual methods in the class I'm
trying to test
(https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension...),
and especially not overriding them inside the test. It's way too easy to
accidentally wind up testing nothing.

I prefer to "design for testing" by designing pure virtual interfaces (or
classes with some pure virtual functions and other non-virtual helper
functions), and then passing mock or fake instances of those classes to the
constructor of the class I'm trying to test. It's still possible to wind up
testing a very thin function with a mock instance under it, but then it's much
more obvious when looking at the production code that this class or function
isn't doing anything.

+  EXPECT_EQ(arraysize(kTestingDisplayInfoData), display_info.size());
   for (size_t i = 0; i < arraysize(kTestingDisplayInfoData); ++i) {
     const linked_ptr<api::system_info_display::DisplayUnitInfo> unit =
-        (*display_info.get())[i];
+        display_info[i];
     EXPECT_EQ(kTestingDisplayInfoData[i].id, unit->id);
     EXPECT_EQ(kTestingDisplayInfoData[i].name, unit->name);
     EXPECT_DOUBLE_EQ(kTestingDisplayInfoData[i].dpi_x, unit->dpi_x);
     EXPECT_DOUBLE_EQ(kTestingDisplayInfoData[i].dpi_y, unit->dpi_y);
     EXPECT_EQ(kTestingDisplayInfoData[i].is_enabled, unit->is_enabled);
     EXPECT_EQ(kTestingDisplayInfoData[i].is_internal, unit->is_internal);
     EXPECT_EQ(kTestingDisplayInfoData[i].is_primary, unit->is_primary);
     EXPECT_EQ(kTestingDisplayInfoData[i].bounds.left, unit->bounds.left);
     EXPECT_EQ(kTestingDisplayInfoData[i].bounds.top, unit->bounds.top);
     EXPECT_EQ(kTestingDisplayInfoData[i].bounds.width, unit->bounds.width);
     EXPECT_EQ(kTestingDisplayInfoData[i].bounds.height, unit->bounds.height);
     EXPECT_EQ(kTestingDisplayInfoData[i].work_area.left, unit->work_area.left);
     EXPECT_EQ(kTestingDisplayInfoData[i].work_area.top, unit->work_area.top);
     EXPECT_EQ(kTestingDisplayInfoData[i].work_area.width,
               unit->work_area.width);
     EXPECT_EQ(kTestingDisplayInfoData[i].work_area.height,
               unit->work_area.height);
   }
 }
 
 } // namespace extensions