Geolocation: cleanup wifi_data_provider_common_unittest

Two of the test cases were flakily timing out so this CL rewrites
the threading model of the test, removing the |run_loop_|
and expecting MockWlanApi::GetAccessPointData() to
explicitly finish the tests.

Also this CL:
- simplifies the logic using GMock mojo
- moves a few variables to ctor initializer lists ISO SetUp()
- removes RunNormal() test case bc is a dupe of
 DoAnEmptyScan().

Important: this CL also removes the test case:
GeolocationWifiDataProviderCommonTest::RegisterUnregister

because it didn't belong to this unit test; incidentally
it also verifies little since it just register-unregisters a
test factory.  Will land some specific tests for this class
very soon.

BUG=405596

Review-Url: https://codereview.chromium.org/2829983002
Cr-Commit-Position: refs/heads/master@{#466525}

device/geolocation/wifi_data_provider_common.cc

@@ -13,7 +13,7 @@
 namespace device {
 
 base::string16 MacAddressAsString16(const uint8_t mac_as_int[6]) {
-  // mac_as_int is big-endian. Write in byte chunks.
+  // |mac_as_int| is big-endian. Write in byte chunks.
   // Format is XX-XX-XX-XX-XX-XX.
   static const char* const kMacFormatString = "%02x-%02x-%02x-%02x-%02x-%02x";
   return base::ASCIIToUTF16(base::StringPrintf(
@@ -27,17 +27,17 @@ WifiDataProviderCommon::WifiDataProviderCommon()
 WifiDataProviderCommon::~WifiDataProviderCommon() {}
 
 void WifiDataProviderCommon::StartDataProvider() {
-  DCHECK(wlan_api_ == NULL);
+  DCHECK(!wlan_api_);
   wlan_api_.reset(NewWlanApi());
-  if (wlan_api_ == NULL) {
+  if (!wlan_api_) {
     // Error! Can't do scans, so don't try and schedule one.
     is_first_scan_complete_ = true;
     return;
   }
 
-  DCHECK(polling_policy_ == NULL);
+  DCHECK(!polling_policy_);
   polling_policy_.reset(NewPollingPolicy());
-  DCHECK(polling_policy_ != NULL);
+  DCHECK(polling_policy_);
 
   // Perform first scan ASAP regardless of the polling policy. If this scan
   // fails we'll retry at a rate in line with the polling policy.

device/geolocation/wifi_data_provider_common.h

@@ -50,9 +50,10 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderCommon
  protected:
   ~WifiDataProviderCommon() override;
 
+  // TODO(mcasas): change return types and possibly names of these two methods,
+  // see https://crbug.com/714348.
   // Returns ownership.
   virtual WlanApiInterface* NewWlanApi() = 0;
-
   // Returns ownership.
   virtual WifiPollingPolicy* NewPollingPolicy() = 0;
 

device/geolocation/wifi_data_provider_common_unittest.cc

@@ -17,211 +17,177 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::AnyNumber;
 using testing::AtLeast;
-using testing::DoDefault;
+using testing::DoAll;
 using testing::Invoke;
+using testing::InvokeWithoutArgs;
 using testing::Return;
+using testing::SetArgPointee;
+using testing::WithArgs;
 
 namespace device {
 
 class MockWlanApi : public WifiDataProviderCommon::WlanApiInterface {
  public:
-  MockWlanApi() : calls_(0), bool_return_(true) {
-    ANNOTATE_BENIGN_RACE(&calls_, "This is a test-only data race on a counter");
+  MockWlanApi() {
     ON_CALL(*this, GetAccessPointData(_))
-        .WillByDefault(Invoke(this, &MockWlanApi::GetAccessPointDataInternal));
+        .WillByDefault(DoAll(SetArgPointee<0>(data_out_), Return(true)));
   }
 
   MOCK_METHOD1(GetAccessPointData, bool(WifiData::AccessPointDataSet* data));
 
-  int calls_;
-  bool bool_return_;
-  WifiData::AccessPointDataSet data_out_;
-
  private:
-  bool GetAccessPointDataInternal(WifiData::AccessPointDataSet* data) {
-    ++calls_;
-    *data = data_out_;
-    return bool_return_;
-  }
+  WifiData::AccessPointDataSet data_out_;
 };
 
 class MockPollingPolicy : public WifiPollingPolicy {
  public:
   MockPollingPolicy() {
     ON_CALL(*this, PollingInterval()).WillByDefault(Return(1));
     ON_CALL(*this, NoWifiInterval()).WillByDefault(Return(1));
+    // We are not interested in calls to UpdatePollingInterval() method.
+    EXPECT_CALL(*this, UpdatePollingInterval(_)).Times(AnyNumber());
   }
 
+  // WifiPollingPolicy implementation.
+  MOCK_METHOD1(UpdatePollingInterval, void(bool));
   MOCK_METHOD0(PollingInterval, int());
   MOCK_METHOD0(NoWifiInterval, int());
-
-  virtual void UpdatePollingInterval(bool) {}
 };
 
 class WifiDataProviderCommonWithMock : public WifiDataProviderCommon {
  public:
   WifiDataProviderCommonWithMock()
-      : new_wlan_api_(new MockWlanApi),
-        new_polling_policy_(new MockPollingPolicy) {}
+      : wlan_api_(new MockWlanApi), polling_policy_(new MockPollingPolicy) {}
 
   // WifiDataProviderCommon
-  WlanApiInterface* NewWlanApi() override {
-    CHECK(new_wlan_api_ != NULL);
-    return new_wlan_api_.release();
-  }
+  WlanApiInterface* NewWlanApi() override { return wlan_api_.release(); }
   WifiPollingPolicy* NewPollingPolicy() override {
-    CHECK(new_polling_policy_ != NULL);
-    return new_polling_policy_.release();
+    return polling_policy_.release();
   }
 
-  std::unique_ptr<MockWlanApi> new_wlan_api_;
-  std::unique_ptr<MockPollingPolicy> new_polling_policy_;
+  std::unique_ptr<MockWlanApi> wlan_api_;
+  std::unique_ptr<MockPollingPolicy> polling_policy_;
 
  private:
   ~WifiDataProviderCommonWithMock() override {}
 
   DISALLOW_COPY_AND_ASSIGN(WifiDataProviderCommonWithMock);
 };
 
-WifiDataProvider* CreateWifiDataProviderCommonWithMock() {
-  return new WifiDataProviderCommonWithMock;
-}
-
 // Main test fixture
 class GeolocationWifiDataProviderCommonTest : public testing::Test {
  public:
   GeolocationWifiDataProviderCommonTest()
-      : main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-        wifi_data_callback_(
-            base::Bind(&GeolocationWifiDataProviderCommonTest::OnWifiDataUpdate,
-                       base::Unretained(this))) {}
+      : wifi_data_callback_(base::Bind(&base::DoNothing)),
+        provider_(new WifiDataProviderCommonWithMock),
+        wlan_api_(provider_->wlan_api_.get()),
+        polling_policy_(provider_->polling_policy_.get()) {}
 
   void SetUp() override {
-    provider_ = new WifiDataProviderCommonWithMock;
-    wlan_api_ = provider_->new_wlan_api_.get();
-    polling_policy_ = provider_->new_polling_policy_.get();
     provider_->AddCallback(&wifi_data_callback_);
   }
 
   void TearDown() override {
     provider_->RemoveCallback(&wifi_data_callback_);
     provider_->StopDataProvider();
-    provider_ = NULL;
-  }
-
-  void OnWifiDataUpdate() {
-    // Callbacks must run on the originating thread.
-    EXPECT_TRUE(main_task_runner_->BelongsToCurrentThread());
-    run_loop_->Quit();
-  }
-
-  void RunLoop() {
-    run_loop_.reset(new base::RunLoop());
-    run_loop_->Run();
   }
 
  protected:
-  base::MessageLoopForUI message_loop_;
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
-  std::unique_ptr<base::RunLoop> run_loop_;
+  const base::MessageLoopForUI message_loop_;
   WifiDataProviderManager::WifiDataUpdateCallback wifi_data_callback_;
-  scoped_refptr<WifiDataProviderCommonWithMock> provider_;
-  MockWlanApi* wlan_api_;
-  MockPollingPolicy* polling_policy_;
+  const scoped_refptr<WifiDataProviderCommonWithMock> provider_;
+
+  MockWlanApi* const wlan_api_;
+  MockPollingPolicy* const polling_policy_;
 };
 
 TEST_F(GeolocationWifiDataProviderCommonTest, CreateDestroy) {
   // Test fixture members were SetUp correctly.
-  EXPECT_TRUE(main_task_runner_->BelongsToCurrentThread());
-  EXPECT_TRUE(NULL != provider_.get());
-  EXPECT_TRUE(NULL != wlan_api_);
-}
-
-TEST_F(GeolocationWifiDataProviderCommonTest, RunNormal) {
-  EXPECT_CALL(*wlan_api_, GetAccessPointData(_)).Times(AtLeast(1));
-  EXPECT_CALL(*polling_policy_, PollingInterval()).Times(AtLeast(1));
-  provider_->StartDataProvider();
-  RunLoop();
-  SUCCEED();
+  EXPECT_TRUE(provider_);
+  EXPECT_TRUE(wlan_api_);
+  EXPECT_TRUE(polling_policy_);
 }
 
 TEST_F(GeolocationWifiDataProviderCommonTest, NoWifi) {
+  base::RunLoop run_loop;
   EXPECT_CALL(*polling_policy_, NoWifiInterval()).Times(AtLeast(1));
-  EXPECT_CALL(*wlan_api_, GetAccessPointData(_)).WillRepeatedly(Return(false));
+  EXPECT_CALL(*wlan_api_, GetAccessPointData(_))
+      .WillOnce(InvokeWithoutArgs([&run_loop]() {
+        run_loop.Quit();
+        return false;
+      }));
+
   provider_->StartDataProvider();
-  RunLoop();
+  run_loop.Run();
 }
 
 TEST_F(GeolocationWifiDataProviderCommonTest, IntermittentWifi) {
+  base::RunLoop run_loop;
   EXPECT_CALL(*polling_policy_, PollingInterval()).Times(AtLeast(1));
   EXPECT_CALL(*polling_policy_, NoWifiInterval()).Times(1);
   EXPECT_CALL(*wlan_api_, GetAccessPointData(_))
       .WillOnce(Return(true))
-      .WillOnce(Return(false))
-      .WillRepeatedly(DoDefault());
-
-  AccessPointData single_access_point;
-  single_access_point.channel = 2;
-  single_access_point.mac_address = 3;
-  single_access_point.radio_signal_strength = 4;
-  single_access_point.signal_to_noise = 5;
-  single_access_point.ssid = base::ASCIIToUTF16("foossid");
-  wlan_api_->data_out_.insert(single_access_point);
+      .WillOnce(InvokeWithoutArgs([&run_loop]() {
+        run_loop.Quit();
+        return false;
+      }));
 
   provider_->StartDataProvider();
-  RunLoop();
-  RunLoop();
+  run_loop.Run();
 }
 
-#if defined(OS_MACOSX)
-#define MAYBE_DoAnEmptyScan DISABLED_DoAnEmptyScan
-#else
-#define MAYBE_DoAnEmptyScan DoAnEmptyScan
-#endif
-TEST_F(GeolocationWifiDataProviderCommonTest, MAYBE_DoAnEmptyScan) {
-  EXPECT_CALL(*wlan_api_, GetAccessPointData(_)).Times(AtLeast(1));
+// This test runs StartDataProvider() and expects that GetAccessPointData() is
+// called. The retrieved WifiData is expected to be empty.
+TEST_F(GeolocationWifiDataProviderCommonTest, DoAnEmptyScan) {
+  base::RunLoop run_loop;
+
   EXPECT_CALL(*polling_policy_, PollingInterval()).Times(AtLeast(1));
+  EXPECT_CALL(*wlan_api_, GetAccessPointData(_))
+      .WillOnce(InvokeWithoutArgs([&run_loop]() {
+        run_loop.Quit();
+        return true;
+      }));
+
   provider_->StartDataProvider();
-  RunLoop();
-  EXPECT_EQ(wlan_api_->calls_, 1);
+  run_loop.Run();
+
   WifiData data;
   EXPECT_TRUE(provider_->GetData(&data));
-  EXPECT_EQ(0, static_cast<int>(data.access_point_data.size()));
+  EXPECT_TRUE(data.access_point_data.empty());
 }
 
-#if defined(OS_MACOSX)
-#define MAYBE_DoScanWithResults DISABLED_DoScanWithResults
-#else
-#define MAYBE_DoScanWithResults DoScanWithResults
-#endif
-TEST_F(GeolocationWifiDataProviderCommonTest, MAYBE_DoScanWithResults) {
-  EXPECT_CALL(*wlan_api_, GetAccessPointData(_)).Times(AtLeast(1));
+// This test runs StartDataProvider() and expects that GetAccessPointData() is
+// called. Some mock WifiData is returned then and expected to be retrieved.
+TEST_F(GeolocationWifiDataProviderCommonTest, DoScanWithResults) {
+  base::RunLoop run_loop;
+
   EXPECT_CALL(*polling_policy_, PollingInterval()).Times(AtLeast(1));
   AccessPointData single_access_point;
   single_access_point.channel = 2;
   single_access_point.mac_address = 3;
   single_access_point.radio_signal_strength = 4;
   single_access_point.signal_to_noise = 5;
   single_access_point.ssid = base::ASCIIToUTF16("foossid");
-  wlan_api_->data_out_.insert(single_access_point);
+
+  WifiData::AccessPointDataSet data_out({single_access_point});
+
+  EXPECT_CALL(*wlan_api_, GetAccessPointData(_))
+      .WillOnce(WithArgs<0>(
+          Invoke([&data_out, &run_loop](WifiData::AccessPointDataSet* data) {
+            *data = data_out;
+            run_loop.Quit();
+            return true;
+          })));
 
   provider_->StartDataProvider();
-  RunLoop();
-  EXPECT_EQ(wlan_api_->calls_, 1);
+  run_loop.Run();
+
   WifiData data;
   EXPECT_TRUE(provider_->GetData(&data));
-  EXPECT_EQ(1, static_cast<int>(data.access_point_data.size()));
+  ASSERT_EQ(1u, data.access_point_data.size());
   EXPECT_EQ(single_access_point.ssid, data.access_point_data.begin()->ssid);
 }
 
-TEST_F(GeolocationWifiDataProviderCommonTest, RegisterUnregister) {
-  WifiDataProviderManager::SetFactoryForTesting(
-      CreateWifiDataProviderCommonWithMock);
-  WifiDataProviderManager::Register(&wifi_data_callback_);
-  RunLoop();
-  WifiDataProviderManager::Unregister(&wifi_data_callback_);
-  WifiDataProviderManager::ResetFactoryForTesting();
-}
-
 }  // namespace device