Commit 81b2a7ce authored by Victor Hugo Vianna Silva's avatar Victor Hugo Vianna Silva Committed by Commit Bot

Split PSS sign-out unit tests and fix StopAndClear misconception

This CL has 2 goals.

1) Split EnableSyncSignOutAndChangeAccount into smaller tests so that
when the linked bug is fixed, we have a good place to put our
expectation that IsSyncRequested gets cleared on sign-out. The existing
test apparently was even testing some invalidations code [1].

2) Stop coupling StopAndClear() with sign-out in documentation/comments,
since this isn't super true nowadays. For example on Desktop, when the
user chooses to disable sync by sign-out + clear all data, the
StopAndClear() call is rather a by-product of deleting the profile [2],
not of clearing the primary account on the identity manager.

As a bonus, we update some references in comments to RequestStop(), the
old name of StopAndClear().

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1164954/52/components/browser_sync/profile_sync_service_unittest.cc#707
[2] https://source.chromium.org/chromium/chromium/src/+/a5edbb26dc14b07795e7c45fa15998a4dc7f7fad:chrome/browser/ui/webui/settings/people_handler.cc;l=706

Bug: 1147026
Change-Id: I1d9dc5af66a7e58a52d6cf0f3942db315926acfd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530029
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826227}
parent 57b86546
...@@ -350,10 +350,10 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, FastEnableDisableEnable) { ...@@ -350,10 +350,10 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, FastEnableDisableEnable) {
} }
} }
// This test makes sure that after a RequestStop(CLEAR_DATA), Sync data gets // This test makes sure that after a StopAndClear(), Sync data gets redownloaded
// redownloaded when Sync is started again. This does not actually verify that // when Sync is started again. This does not actually verify that the data is
// the data is gone from disk (which seems infeasible); it's mostly here as a // gone from disk (which seems infeasible); it's mostly here as a baseline for
// baseline for the following tests. // the following tests.
IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
RedownloadsAfterClearData) { RedownloadsAfterClearData) {
ASSERT_TRUE(SetupClients()); ASSERT_TRUE(SetupClients());
......
...@@ -319,9 +319,10 @@ class ProfileSyncService : public SyncService, ...@@ -319,9 +319,10 @@ class ProfileSyncService : public SyncService,
const std::string& message, const std::string& message,
UnrecoverableErrorReason reason); UnrecoverableErrorReason reason);
// Stops the sync engine. Does NOT set IsSyncRequested to false. Use // Stops the sync engine. |data_fate| controls whether the local sync data is
// RequestStop for that. |data_fate| controls whether the local sync data is
// deleted or kept when the engine shuts down. // deleted or kept when the engine shuts down.
// Does NOT set IsSyncRequested to false, use StopAndClear() or
// SyncUserSettings::SetSyncRequested() for that.
void StopImpl(SyncStopDataFate data_fate); void StopImpl(SyncStopDataFate data_fate);
// Puts the engine's sync scheduler into NORMAL mode. // Puts the engine's sync scheduler into NORMAL mode.
......
...@@ -648,36 +648,81 @@ TEST_F(ProfileSyncServiceTest, DisableAndEnableSyncTemporarily) { ...@@ -648,36 +648,81 @@ TEST_F(ProfileSyncServiceTest, DisableAndEnableSyncTemporarily) {
} }
// Certain ProfileSyncService tests don't apply to Chrome OS, for example // Certain ProfileSyncService tests don't apply to Chrome OS, for example
// things that deal with concepts like "signing out" and policy. // things that deal with concepts like "signing out".
#if !defined(OS_CHROMEOS) #if !defined(OS_CHROMEOS)
TEST_F(ProfileSyncServiceTest, EnableSyncSignOutAndChangeAccount) { TEST_F(ProfileSyncServiceTest, SignOutDisablesSyncTransportAndSyncFeature) {
// Sign-in and enable sync.
CreateService(ProfileSyncService::AUTO_START); CreateService(ProfileSyncService::AUTO_START);
SignIn(); SignIn();
InitializeForNthSync(); InitializeForNthSync();
ASSERT_EQ(SyncService::DisableReasonSet(), service()->GetDisableReasons());
EXPECT_EQ(SyncService::DisableReasonSet(), service()->GetDisableReasons()); ASSERT_EQ(SyncService::TransportState::ACTIVE,
EXPECT_EQ(SyncService::TransportState::ACTIVE,
service()->GetTransportState()); service()->GetTransportState());
EXPECT_EQ(identity_manager()->GetPrimaryAccountId(),
identity_provider()->GetActiveAccountId());
// Sign-out.
auto* account_mutator = identity_manager()->GetPrimaryAccountMutator(); auto* account_mutator = identity_manager()->GetPrimaryAccountMutator();
DCHECK(account_mutator) << "Account mutator should only be null on ChromeOS.";
// GetPrimaryAccountMutator() returns nullptr on ChromeOS only.
DCHECK(account_mutator);
account_mutator->ClearPrimaryAccount( account_mutator->ClearPrimaryAccount(
signin::PrimaryAccountMutator::ClearAccountsAction::kDefault, signin::PrimaryAccountMutator::ClearAccountsAction::kDefault,
signin_metrics::SIGNOUT_TEST, signin_metrics::SIGNOUT_TEST,
signin_metrics::SignoutDelete::IGNORE_METRIC); signin_metrics::SignoutDelete::IGNORE_METRIC);
// Wait for PSS to be notified that the primary account has gone away. // Wait for ProfileSyncService to be notified.
base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle();
EXPECT_EQ( EXPECT_EQ(
SyncService::DisableReasonSet(SyncService::DISABLE_REASON_NOT_SIGNED_IN), SyncService::DisableReasonSet(SyncService::DISABLE_REASON_NOT_SIGNED_IN),
service()->GetDisableReasons()); service()->GetDisableReasons());
EXPECT_EQ(SyncService::TransportState::DISABLED, EXPECT_EQ(SyncService::TransportState::DISABLED,
service()->GetTransportState()); service()->GetTransportState());
}
TEST_F(ProfileSyncServiceTest,
SignOutClearsSyncTransportDataAndSyncTheFeaturePrefs) {
// Sign-in and enable sync.
CreateService(ProfileSyncService::AUTO_START);
SignIn();
InitializeForNthSync();
ASSERT_TRUE(service()->GetUserSettings()->IsFirstSetupComplete());
ASSERT_TRUE(service()->GetUserSettings()->IsSyncRequested());
// Sign-out.
auto* account_mutator = identity_manager()->GetPrimaryAccountMutator();
DCHECK(account_mutator) << "Account mutator should only be null on ChromeOS.";
EXPECT_CALL(*sync_client(), OnLocalSyncTransportDataCleared())
.Times(testing::AtLeast(1));
account_mutator->ClearPrimaryAccount(
signin::PrimaryAccountMutator::ClearAccountsAction::kDefault,
signin_metrics::SIGNOUT_TEST,
signin_metrics::SignoutDelete::IGNORE_METRIC);
// Wait for ProfileSyncService to be notified.
base::RunLoop().RunUntilIdle();
// These are specific to sync-the-feature and should be cleared.
EXPECT_FALSE(service()->GetUserSettings()->IsFirstSetupComplete());
// TODO(crbug.com/1147026): Add expectation for IsSyncRequested when it's
// being cleared.
}
TEST_F(ProfileSyncServiceTest, IdentityProvider_GetActiveAccountId) {
// Sign-in and enable sync.
CreateService(ProfileSyncService::AUTO_START);
SignIn();
InitializeForNthSync();
EXPECT_EQ(identity_manager()->GetPrimaryAccountId(),
identity_provider()->GetActiveAccountId());
// Sign out.
auto* account_mutator = identity_manager()->GetPrimaryAccountMutator();
DCHECK(account_mutator) << "Account mutator should only be null on ChromeOS.";
account_mutator->ClearPrimaryAccount(
signin::PrimaryAccountMutator::ClearAccountsAction::kDefault,
signin_metrics::SIGNOUT_TEST,
signin_metrics::SignoutDelete::IGNORE_METRIC);
// Wait for ProfileSyncService to be notified.
base::RunLoop().RunUntilIdle();
// The identity provider should show no active account.
EXPECT_EQ(CoreAccountId(), identity_provider()->GetActiveAccountId()); EXPECT_EQ(CoreAccountId(), identity_provider()->GetActiveAccountId());
// Change account.
identity_test_env()->MakePrimaryAccountAvailable("new_user@gmail.com"); identity_test_env()->MakePrimaryAccountAvailable("new_user@gmail.com");
EXPECT_EQ(identity_manager()->GetPrimaryAccountId(), EXPECT_EQ(identity_manager()->GetPrimaryAccountId(),
identity_provider()->GetActiveAccountId()); identity_provider()->GetActiveAccountId());
...@@ -938,8 +983,8 @@ TEST_F(ProfileSyncServiceTest, SignOutRevokeAccessToken) { ...@@ -938,8 +983,8 @@ TEST_F(ProfileSyncServiceTest, SignOutRevokeAccessToken) {
} }
#endif #endif
// Verify that prefs are cleared on sign-out. TEST_F(ProfileSyncServiceTest,
TEST_F(ProfileSyncServiceTest, ClearDataOnSignOut) { StopAndClearWillClearDataAndSwitchToTransportMode) {
SignIn(); SignIn();
CreateService(ProfileSyncService::AUTO_START); CreateService(ProfileSyncService::AUTO_START);
InitializeForNthSync(); InitializeForNthSync();
...@@ -949,15 +994,14 @@ TEST_F(ProfileSyncServiceTest, ClearDataOnSignOut) { ...@@ -949,15 +994,14 @@ TEST_F(ProfileSyncServiceTest, ClearDataOnSignOut) {
ASSERT_LT(base::Time::Now() - last_synced_time, ASSERT_LT(base::Time::Now() - last_synced_time,
base::TimeDelta::FromMinutes(1)); base::TimeDelta::FromMinutes(1));
// Local transport data should be cleared and the client notified.
EXPECT_CALL(*sync_client(), OnLocalSyncTransportDataCleared()) EXPECT_CALL(*sync_client(), OnLocalSyncTransportDataCleared())
.Times(testing::AtLeast(1)); .Times(testing::AtLeast(1));
// Sign out.
service()->StopAndClear(); service()->StopAndClear();
// Even though Sync-the-feature is disabled, Sync-the-transport should still // Even though Sync-the-feature is disabled, there's still an (unconsented)
// be running, and should have updated the last synced time. // signed-in account, so Sync-the-transport should still be running and
// should have updated the last synced time.
EXPECT_EQ(SyncService::TransportState::ACTIVE, EXPECT_EQ(SyncService::TransportState::ACTIVE,
service()->GetTransportState()); service()->GetTransportState());
EXPECT_FALSE(service()->IsSyncFeatureEnabled()); EXPECT_FALSE(service()->IsSyncFeatureEnabled());
...@@ -978,7 +1022,7 @@ TEST_F(ProfileSyncServiceTest, ClearDemographicsOnInitializeWhenSignedOut) { ...@@ -978,7 +1022,7 @@ TEST_F(ProfileSyncServiceTest, ClearDemographicsOnInitializeWhenSignedOut) {
InitializeForNthSync(); InitializeForNthSync();
} }
TEST_F(ProfileSyncServiceTest, CancelSyncAfterSignOut) { TEST_F(ProfileSyncServiceTest, StopSyncAndClearTwiceDoesNotCrash) {
SignIn(); SignIn();
CreateService(ProfileSyncService::AUTO_START); CreateService(ProfileSyncService::AUTO_START);
InitializeForNthSync(); InitializeForNthSync();
......
...@@ -104,14 +104,14 @@ cr.define('chrome.sync', function() { ...@@ -104,14 +104,14 @@ cr.define('chrome.sync', function() {
} }
/** /**
* Triggers a RequestStop(KEEP_DATA) call on the SyncService. * Stops the SyncService while keeping the sync data around.
*/ */
function requestStopKeepData() { function requestStopKeepData() {
chrome.send('requestStopKeepData'); chrome.send('requestStopKeepData');
} }
/** /**
* Triggers a RequestStop(CLEAR_DATA) call on the SyncService. * Stops the SyncService and clears the sync data.
*/ */
function requestStopClearData() { function requestStopClearData() {
chrome.send('requestStopClearData'); chrome.send('requestStopClearData');
......
...@@ -319,10 +319,8 @@ class SyncService : public KeyedService { ...@@ -319,10 +319,8 @@ class SyncService : public KeyedService {
// ACTIONS / STATE CHANGE REQUESTS // ACTIONS / STATE CHANGE REQUESTS
////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////
// Stops sync and clears all local data. This usually gets called when the // Stops sync-the-feature and clears all local data. Sync-the-transport may
// user fully signs out (i.e. removes the primary account). // remain active after calling this.
// Note: This refers to Sync-the-feature. Sync-the-transport may remain active
// after calling this.
virtual void StopAndClear() = 0; virtual void StopAndClear() = 0;
// Called when a datatype (SyncableService) has a need for sync to start // Called when a datatype (SyncableService) has a need for sync to start
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment