Need this one to unblock  crbug.com/890813 , taking it...

@sdefresne I've spent some time on this one today and, while the implementation is straightforward (one liner), writing the unit test is proving to be difficult, since there's no easy way that I can find to check that an account has been merged into the Cookie jar after calling this method.

I tried to see what was done to test the current method in SigninManager, but it happens not to be tested, so I figured new infrastructure would be needed. With this in mind, I added a |GetCookieAccounts| (bad name, only meant for testing for now) to identity_test_utils and identity_test_environment as a way to retrieve those the account IDs of the merged accounts from the Cookie jar after invoking the method, but run into several problems, such as:

  * The SigninManager instance created by IdentityManagerDependenciesOwner didn't include the GaiaCookieManagerService, so calls to MergeSigninCredentialIntoCookieJar() would crash unless I fixed that (see the current WIP patch).

  * Calling MergeSigninCredentialIntoCookieJar() would result in GaiaCookieManagerService::AddAccountToCookie() to be called, which will trigger a request for an access token, so had to add extra code in the test to wait for it.

  * Even if I add observers in the test for the identity manager and/or the GaiaCookieManagerService (had to add a method in IdentityTestEnvironment to get the internal cookie service first in this case), I'm not sure that calls to MergeSigninCredentialIntoCookieJar() are working as expected, because I can't get any of the cookie-related callbacks from IdentityManager::Observer or GaiaCookieManagerService::Observer being called (even if I run them in a nested RunLoop).


So, after reaching this point I think I'd better ask for some clarification / hints in case I'm missing something obvious. To expose more easily what I did so far, you can see my WIP CL in https://chromium-review.googlesource.com/c/chromium/src/+/1355192.

Thanks in advance!

I don't have a ready answer. I am going to take a look at whether there is an easy fix to have the test working. If not, given that the method is legacy and there is no tests in SigninManager, I would recommend landing this without any tests (this won't be worse than the state we currently are).

I'll come back by beginning of day tomorrow or earlier.

I did look in detail, and it appears that many more network requests are required to implement GaiaCookieManagerService::AddAccountToCookie(). Your CL only reply to the first of those request, but then a new one is started from UbertokenFetcher::ExchangeTokens() and it looks like it could be difficult to inject fake reply for this.

Let's just land the implementation for this method without unit tests as it may prove really difficult to test (we are in no worse situation than before since the method implementation is straight-forward so cannot have bug and the underlying feature is already untested).

@sdefresne Thanks for the careful analysis, looks like my suspicions on that this was really difficult to test were not unfounded :-). I agree with you, will change the CL to only include the implementation this time, leaving the unit tests aside for once.

CL here, btw: https://chromium-review.googlesource.com/c/chromium/src/+/1355192

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

commit 015dd59b45e26ee39d5750a71f7800fde98c7c6f
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Dec 05 10:54:48 2018

Implement PrimaryAccountMutator::LegacyMergeSigninCredentialIntoCookieJar()

The implementation is straightforward, but the test for this is difficult
to write due to how GaiaCookieManagerService::AddAccountToCookie(), which
gets called when invoking MergeSigninCredentialIntoCookieJar(), fires up
several network requests we'd need to inject fake replies for in order to
be able to properly test this functionality.

Thus, considering that this is a legacy method which we already didn't
have unit tests for, and that the implementation is just a one-line wrapper
of the same untested functionality in SigninManager, we're landing this
method untested since we won't be in a worse situation than we already are,
but in a better one since landing it would at least unblock some tickets
that we need to migrate to the Identity service.

Bug:  906062 
Change-Id: Ib1ffb2398019ded5e29cf75caf3cb7fae7175ac9
Reviewed-on: https://chromium-review.googlesource.com/c/1355192
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#613932}
[modify] https://crrev.com/015dd59b45e26ee39d5750a71f7800fde98c7c6f/services/identity/public/cpp/primary_account_mutator_impl.cc