New issue
Advanced search Search tips

Issue 910581 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 911641
issue 922019

Blocking:
issue 883318
issue 883330



Sign in to add a comment

Convert account_reconcilor_unittest.cc to IdentityManager

Project Member Reported by toniki...@chromium.org, Nov 30

Issue description

Production code was migrated [1], and unit tests were minimally updated to keep running.

Now it is time to fully migrate the account_reconcilor_unittest.cc code away from SigninManager and PO2TS.

[1] https://chromium-review.googlesource.com/c/1330223
 
Blocking: 883330 883318
Labels: -Pri-2 Pri-1
Blockedon: 911641
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 6

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

commit 0d46e46ee383877ad62974bf266d5bbc96c2f056
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Dec 06 13:09:06 2018

[s13n] Convert account_reconcilor_unittest.cc to IdentityManager

This CL migrates (most of) account_reconcilor_unittest.cc code
away from SigninManager and PO2TS.
Production code was migrated and unit tests were minimally updated
to keep running in [1].

Remarks:
- The CL can completely remove SigninManager direct API calls.
- The CL could not completely remove direct PO2TS API calls, due to two
missing API: set_all_credentials_loaded_for_testing and LoadCredentials.
- Given how the test uses gaia_id to manipulate reconciles and how
IdentityTestEnvironment hides setting the gaia_id from the tests,
there were some adaptations needed so that the functionalities could
continue fully tested. For instance, the following methods were added/
used:

* GaiaIdForAccountKey: Added.
* identity::GetTestGaiaIdForEmail: Referenced.

[1] https://chromium-review.googlesource.com/c/1330223

BUG=910581

Change-Id: I58db6044caa22b6072cb6d22dd7c97c668c46cd3
Reviewed-on: https://chromium-review.googlesource.com/c/1357059
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#614327}
[modify] https://crrev.com/0d46e46ee383877ad62974bf266d5bbc96c2f056/components/signin/core/browser/account_reconcilor_unittest.cc

Hi Antonio,

Do you know if there is a bug for migrating AccountReconcilor away from ProfileOAuth2TokenService? Is this a task that you already had in your queue? Thanks!

Comment 6 Deleted

Hi Colin.

There used to be   bug 903907   (which I have merged a patch for, https://crrev.com/c/1330223), but as you said, it only moved it away from SigninManager. We still need to migrate it away from PO2TS. I have reopened it and retitled it accordingly.
Thank you!
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13

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

commit 745381ba737bfdc1a6f678fbbdb066a1f8045693
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Dec 13 13:45:48 2018

[s13n] Provide replacement API for PO2TS::set_all_credentials_loaded_for_testing

CL is another step forward eliminating all direct calls to PO2TS
APIs from AccountReconcilor unittests. Particularly, it adds a new
API to our testing infrastructure, IdentityTestEnvironment.

BUG= 911641 ,910581

Change-Id: I1ff3a8f287fff0f5b4bfbffe2e2d6943ba36ec52
Reviewed-on: https://chromium-review.googlesource.com/c/1372426
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#616298}
[modify] https://crrev.com/745381ba737bfdc1a6f678fbbdb066a1f8045693/components/signin/core/browser/account_reconcilor_unittest.cc
[modify] https://crrev.com/745381ba737bfdc1a6f678fbbdb066a1f8045693/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/745381ba737bfdc1a6f678fbbdb066a1f8045693/services/identity/public/cpp/identity_test_environment.h

Update: what is missing here in a moving away from a direct call to PO2TS::LoadCredentials:


TEST_P(AccountReconcilorMirrorEndpointParamTest, TokensNotLoaded) {
  const std::string account_id =
      ConnectProfileToAccount("user@gmail.com").account_id;
  cookie_manager_service()->SetListAccountsResponseNoAccounts();
  identity_test_env()->ResetToAccountsNotYetLoadedFromDiskState();

  AccountReconcilor* reconcilor = GetMockReconcilor();
  reconcilor->StartReconcile();

#if !defined(OS_CHROMEOS)
  // No reconcile when tokens are not loaded, except on ChromeOS where reconcile
  // can start as long as the token service is not empty.
  ASSERT_FALSE(reconcilor->is_reconcile_started_);
  // When tokens are loaded, reconcile starts automatically.
  token_service()->LoadCredentials(account_id);

Blockedon: 922019

Sign in to add a comment