[NTP OptOut] Refactoring tests

A promissed follow-up test refactoring, which removes friending of test
classes. It also found a bug, already fixed in original patch.

Bug: 805171
Change-Id: I1d7c3a8043df705f3e21cd7dabdab401b424f08f
Reviewed-on: https://chromium-review.googlesource.com/903586
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536045}

components/ntp_snippets/remote/remote_suggestions_status_service_impl.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_STATUS_SERVICE_IMPL_H_
 
 #include "base/callback.h"
-#include "base/gtest_prod_util.h"
 #include "base/scoped_observer.h"
 #include "components/ntp_snippets/remote/remote_suggestions_status_service.h"
 #include "components/prefs/pref_change_registrar.h"
@@ -32,25 +31,6 @@ class RemoteSuggestionsStatusServiceImpl
   void OnSignInStateChanged(bool has_signed_in) override;
 
  private:
-  // TODO(jkrcal): Rewrite the tests using the public API - observing status
-  // changes instead of calling private GetStatusFromDeps() directly.
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           SigninNeededIfSpecifiedByParam);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           NoSigninNeeded);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           DisabledViaPref);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           DisabledViaAdditionalPref);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           EnabledAfterListFolded);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           DisabledWhenListFoldedOnStart);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           EnablingAfterFoldedStart);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsStatusServiceImplTest,
-                           EnablingAfterFoldedStartSignedIn);
-
   // Callbacks for the PrefChangeRegistrar.
   void OnSnippetsEnabledChanged();
   void OnListVisibilityChanged();

components/ntp_snippets/remote/remote_suggestions_status_service_impl_unittest.cc

@@ -22,14 +22,12 @@ namespace ntp_snippets {
 namespace {
 const char kTestPrefName[] = "search_suggestions.test_name";
 
-void OnStatusChange(RemoteSuggestionsStatus old_status,
-                    RemoteSuggestionsStatus new_status) {}
-
 }  // namespace
 
 class RemoteSuggestionsStatusServiceImplTest : public ::testing::Test {
  public:
-  RemoteSuggestionsStatusServiceImplTest() {
+  RemoteSuggestionsStatusServiceImplTest()
+      : last_status_(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN) {
     RemoteSuggestionsStatusServiceImpl::RegisterProfilePrefs(
         utils_.pref_service()->registry());
 
@@ -45,11 +43,21 @@ class RemoteSuggestionsStatusServiceImplTest : public ::testing::Test {
     auto service = std::make_unique<RemoteSuggestionsStatusServiceImpl>(
         false, utils_.pref_service(),
         list_hiding_enabled ? std::string() : kTestPrefName);
-    service->Init(base::BindRepeating(&OnStatusChange));
+    service->Init(base::BindRepeating(
+        &RemoteSuggestionsStatusServiceImplTest::OnStatusChange,
+        base::Unretained(this)));
     return service;
   }
 
+  RemoteSuggestionsStatus last_status() const { return last_status_; }
+
  protected:
+  void OnStatusChange(RemoteSuggestionsStatus old_status,
+                      RemoteSuggestionsStatus new_status) {
+    last_status_ = new_status;
+  }
+
+  RemoteSuggestionsStatus last_status_;
   test::RemoteSuggestionsTestUtils utils_;
   variations::testing::VariationParamsManager params_manager_;
 };
@@ -58,49 +66,41 @@ TEST_F(RemoteSuggestionsStatusServiceImplTest, NoSigninNeeded) {
   auto service = MakeService(/*list_hiding_enabled=*/false);
 
   // By default, no signin is required.
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // Signin should cause a state change.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest, DisabledViaPref) {
   auto service = MakeService(/*list_hiding_enabled=*/false);
 
   // The default test setup is signed out. The service is enabled.
-  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // Once the enabled pref is set to false, we should be disabled.
   utils_.pref_service()->SetBoolean(prefs::kEnableSnippets, false);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 
   // The state should not change, even though a signin has occurred.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest, DisabledViaAdditionalPref) {
   auto service = MakeService(/*list_hiding_enabled=*/false);
 
   // The default test setup is signed out. The service is enabled.
-  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // Once the additional pref is set to false, we should be disabled.
   utils_.pref_service()->SetBoolean(kTestPrefName, false);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 
   // The state should not change, even though a signin has occurred.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest, EnabledAfterListFolded) {
@@ -109,19 +109,16 @@ TEST_F(RemoteSuggestionsStatusServiceImplTest, EnabledAfterListFolded) {
   EXPECT_TRUE(utils_.pref_service()->GetBoolean(prefs::kArticlesListVisible));
 
   // The default test setup is signed out. The service is enabled.
-  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  ASSERT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // When the user toggles the visibility of articles list in UI off the service
   // should still be enabled until the end of the session.
   utils_.pref_service()->SetBoolean(prefs::kArticlesListVisible, false);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // Signin should cause a state change.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest, DisabledWhenListFoldedOnStart) {
@@ -129,13 +126,11 @@ TEST_F(RemoteSuggestionsStatusServiceImplTest, DisabledWhenListFoldedOnStart) {
   auto service = MakeService(/*list_hiding_enabled="*/ true);
 
   // The state should be disabled when starting with no list shown.
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 
   // The state should not change, even though a signin has occurred.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest, EnablingAfterFoldedStart) {
@@ -143,19 +138,16 @@ TEST_F(RemoteSuggestionsStatusServiceImplTest, EnablingAfterFoldedStart) {
   auto service = MakeService(/*list_hiding_enabled="*/ true);
 
   // The state should be disabled when starting with no list shown.
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 
   // When the user toggles the visibility of articles list in UI on, the service
   // should get enabled.
   utils_.pref_service()->SetBoolean(prefs::kArticlesListVisible, true);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT, last_status());
 
   // Signin should cause a state change.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN, last_status());
 }
 
 TEST_F(RemoteSuggestionsStatusServiceImplTest,
@@ -165,14 +157,12 @@ TEST_F(RemoteSuggestionsStatusServiceImplTest,
 
   // Signin should not cause a state change, because UI is not visible.
   service->OnSignInStateChanged(/*has_signed_in=*/true);
-  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, last_status());
 
   // When the user toggles the visibility of articles list in UI on, the service
   // should get enabled.
   utils_.pref_service()->SetBoolean(prefs::kArticlesListVisible, true);
-  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
-            service->GetStatusFromDeps());
+  EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN, last_status());
 }
 
 }  // namespace ntp_snippets