cc/ blundell

Challenge here seems to be 

142 class MockSigninManager : public SigninManager {
(...)
154 
155   MOCK_METHOD4(OnSignoutDecisionReached,
156                void(signin_metrics::ProfileSignout,
157                     signin_metrics::SignoutDelete,
158                     RemoveAccountsOption remove_option,
159                     SigninClient::SignoutDecision signout_decision));
160 

and how 'OnSignoutDecisionReached' call is verified to be called:

206   EXPECT_CALL(*manager_,
207               OnSignoutDecisionReached(
208                   source_metric, delete_metric,
209                   SigninManager::RemoveAccountsOption::kRemoveAllAccounts,
210                   SigninClient::SignoutDecision::ALLOW_SIGNOUT))
211       .Times(1);

Colin, Sylvain, WDYT?

Hi Antonio,

Thanks for flagging this! It looks to me like we should be able to replace all the calls to SigninManager::SignOut() in the test with calls to ChromeSigninClient::PreSignOut(), passing a MockCallback directly. It looks like there are 6 calls, so this shouldn't be too onerous. Independent of the conversion to IdentityManager, I actually like this change as the current test code is making assumptions on the internal flow of SigninManager::SignOut(), which seems a non-unit-test-ish.

To likely state the obvious, I would do this change in its own precursor CL.

WDYT?

Hi Colin, I believe your idea makes sense, yes.

Please see https://crrev.com/c/1394663 . It is still WIP but would be great to hear from about the following:

Basically, the CL is the precursor CL that moves ChromeSigninClientTests from using SigninManager::SignOut to ChromeSigninClient::PreSignOut.

My doubt is how to handle test ChromeSigninClientSignoutTest.SignOutWithoutManager, where

221   MockSigninManager other_manager(client_.get());
222   other_manager.CopyCredentialsFrom(*manager_.get());

Should I switch to using IdentityTestEnvironment instead of SigninManager as part of this CL, for this specific test?

PS: If so, I believe I would also need instances of AccountTrackerService, GaiaCookieManagerService (SigninManager and PO2TS instances are already available) in order to instantiate IdentityTestEnvironment.

Discussing this question on the CL :).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49d17c6b6d18739bff96db423d32ea3b55368bec

commit 49d17c6b6d18739bff96db423d32ea3b55368bec
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Fri Jan 04 13:35:43 2019

Use ChromeSigninClient::PreSignOut() instead of SigninManager::SignOut() in ChromeSigninClientTests

This CL is step forward to move the test away from SigninManager, PO2TS and AccountTrackerService
completely. The CL:

1) removes the knowledge of the tests about internals of SigninManager (eg that
SigninManager::OnSignoutDecisionReached is called during the SignOut routine).

2) Removes MockSigninManager class completely, after tests switched away to use
ChromeSigninClient::PreSignOut() instead of SigninManager::SignOut().

3) Calls ChromeSigninClient::AfterCredentialsCopied instead of SigninManager::CopyCredentialsFrom
keeping the same behavior.

BUG= 890787 

Change-Id: Ib9dbc5feb073ec7bbda452a680e71de27a361e79
Reviewed-on: https://chromium-review.googlesource.com/c/1394663
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619936}
[modify] https://crrev.com/49d17c6b6d18739bff96db423d32ea3b55368bec/chrome/browser/signin/chrome_signin_client_unittest.cc