[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}diff --git a/components/ntp_snippets/remote/remote_suggestions_status_service_impl.h b/components/ntp_snippets/remote/remote_suggestions_status_service_impl.h
index 06aa81e..ed0234bd 100644
--- a/components/ntp_snippets/remote/remote_suggestions_status_service_impl.h
+++ b/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 @@
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();
diff --git a/components/ntp_snippets/remote/remote_suggestions_status_service_impl_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_status_service_impl_unittest.cc
index eedf9fb..9294376 100644
--- a/components/ntp_snippets/remote/remote_suggestions_status_service_impl_unittest.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_status_service_impl_unittest.cc
@@ -22,14 +22,12 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
// 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
