> The tests should be based on SigninManager test.

@sdefresne Could you ellaborate a little bit more on this? I've started working on this yesterday but I couldn't figure out what you mean by this, since the only example of testing the current SigninManager::AuthInProgress logic is present in IdentityManagerTest instead: 

https://cs.chromium.org/chromium/src/services/identity/public/cpp/identity_manager_unittest.cc?type=cs&q=set_auth_in_progress&sq=package:chromium&g=0&l=709


FWIW, I've wrote a CL in https://chromium-review.googlesource.com/c/chromium/src/+/1342559 with my (maybe wrong) attempt to implement a unit test for this based on what I've seen in IdentityManagerTest, but I'm not convinced on having to add a non-blocking method to IdentityTestEnvironment just to be able to check this... although in the other hand I don't see how I can test it using the other methods, which all block until the task at hand is completed.

If you (or @blundell) have any feedback / hints on any of these 2 topics, I'll appreaciate if you could comment here and/or on such CL. Thanks!

I meant that we should write tests that are at least as good as SigninManagerTest for the same method.

I see. The issue with this particular method is that, as I said, it is not tested at all in SigninManagerTest, just with IdentityManagerTest, and that in order to do a similar test I'd need either direct access to the underlying SigninManager or to add an extra non-blocking method to IdentityTestEnvironment, which I'm not sure it's the right approach (see referenced CL).

If you or @blundell could ellaborate a bit more on this bit, I'd really appreciate. Seems to me like I'm missing something, maybe obvious, here...

Thanks!

I think you should be able to test this method directly if you have access to the other legacy methods (LegacyStartSigninWithRefreshTokenForPrimaryAccount, LegacyCompletePendingPrimaryAccountSignin).

Maybe those three methods (or four if we also include LegacyPrimaryAccountForAuthInProgress method at the same time) should be implemented and tested together.

Note, the tests that I think would be to check that:

1. auth is in progress is false on creation,
2. auth is in progress is true after call to LegacyStartSigninWithRefreshTokenForPrimaryAccount,
3. auth is in progress is false after login complete or is cancelled.

Ah! That makes a lot of sense, thanks for clarifying this. Yes, if I had access to actual implementations of LegacyStartSigninWithRefreshTokenForPrimaryAccount  LegacyCompletePendingPrimaryAccountSignin (and maybe LegacyPrimaryAccountForAuthInProgress as well), testing all this "auth in progress" stuff would be more natural than shoehorning an extra method in IdentityTestEnvironment.

I think I'll try to implement all those at the same time and then add tests for all of them as part of the same CL (or maybe split them in different CLs, one per bug).

I'll comment on the affected bugs, thanks again!

Quick heads-up because I kind of run out of time today while working on bugs  906085  and 908121: I have a local branch with the 4 methods mentioned here implemented and unit tests for the Legacy*AuthInProgress*() ones, I just need to add unit tests for LegacyStartSigninWithRefreshTokenForPrimaryAccount and LegacyCompletePendingPrimaryAccountSignin and the CL here should be ready for review.

However, I need to have LegacyClearPrimaryAccount() implemented first since I'm relying on that for the Legacy*AuthInProgress*() tests (to simulate cancellation of the Auth process), so I need to finish the CL for the blocking bug (906056) before, which I'm planning on doing early next week.

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

commit 02f8a46c06b9b93537cd53fa117223879c7b0546
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Thu Nov 29 12:13:03 2018

Implement legacy methods in PrimaryAccountMutator related to signin process

Implement the following methods at the same time, so that we can also
provide decent unit tests that would require of all of them to make
sure functionality works as expected:

  * LegacyStartSigninWithRefreshTokenForPrimaryAccount()
  * LegacyCompletePendingPrimaryAccountSignin()
  * LegacyIsPrimaryAccountAuthInProgress()
  * LegacyPrimaryAccountForAuthInProgress()

Also, PrimaryAccountMutatorTests unit tests for each of those methods as
part of this CL, based on those from SigninManagerTest, including a new
test for ClearPrimaryAccount (i.e. ClearPrimaryAccount_NotSignedIn) to
check that it reports FALSE when there is neither a primary account nor
an authentication process ongoing (now we can finally implement that).

Bug:  906058 ,  906059 ,  906063 ,  906064 
Change-Id: I2164c867f35020806225da4e69659364c3429a92
Reviewed-on: https://chromium-review.googlesource.com/c/1342559
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612159}
[modify] https://crrev.com/02f8a46c06b9b93537cd53fa117223879c7b0546/services/identity/public/cpp/primary_account_mutator_impl.cc
[modify] https://crrev.com/02f8a46c06b9b93537cd53fa117223879c7b0546/services/identity/public/cpp/primary_account_mutator_unittest.cc