Skip to content

Commit

Permalink
Improve the no-op reconcilor so that it doesn't "hang" after the firs…
Browse files Browse the repository at this point in the history 
…t execution, but instead successfully completes the reconciliation.

Improve the UMAHistogramHelper so that it can nicely take snapshots between tests, allowing the test writer to easily test ONLY the changes logged in "this" test.
Write a new unit test that executes the reconcilor twice, and make all reconcilor unit tests execute with and without the flag.

BUG=357693
TBR=phajdan.jr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277605

Review URL: https://codereview.chromium.org/309843002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278223 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
mlerman@chromium.org committed Jun 19, 2014
1 parent 1399f24 commit 3d08d1c
Show file tree
Hide file tree
Showing 6 changed files with 257 additions and 34 deletions.
4 changes: 4 additions & 0 deletions base/test/statistics_delta_reader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ class HistogramSamples;

// This class acts as a differential reader for histogram samples, enabling
// tests to check that metrics were recorded as they should be.
//
// This class is DEPRECATED.
// TODO(mlerman): Remove all references to this class with UMAHistogramHelper
// references. crbug.com/384011
class StatisticsDeltaReader {
public:
StatisticsDeltaReader();
Expand Down
175 changes: 164 additions & 11 deletions chrome/browser/signin/account_reconcilor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "chrome/test/base/uma_histogram_helper.h"
#include "components/signin/core/browser/account_reconcilor.h"
#include "components/signin/core/browser/profile_oauth2_token_service.h"
#include "components/signin/core/browser/signin_manager.h"
Expand All @@ -34,6 +35,13 @@
namespace {

const char kTestEmail[] = "user@gmail.com";
const char* const kHistogramsToSnapshot[] = {
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun",
"Signin.Reconciler.AddedToCookieJar.FirstRun",
"Signin.Reconciler.AddedToChrome.FirstRun",
"Signin.Reconciler.DifferentPrimaryAccounts.SubsequentRun",
"Signin.Reconciler.AddedToCookieJar.SubsequentRun",
"Signin.Reconciler.AddedToChrome.SubsequentRun"};

class MockAccountReconcilor : public testing::StrictMock<AccountReconcilor> {
public:
Expand Down Expand Up @@ -77,14 +85,15 @@ MockAccountReconcilor::MockAccountReconcilor(

} // namespace

class AccountReconcilorTest : public testing::Test {
class AccountReconcilorTest : public ::testing::TestWithParam<bool> {
public:
AccountReconcilorTest();
virtual void SetUp() OVERRIDE;

TestingProfile* profile() { return profile_; }
FakeSigninManagerForTesting* signin_manager() { return signin_manager_; }
FakeProfileOAuth2TokenService* token_service() { return token_service_; }
UMAHistogramHelper* histogram_helper() { return &histogram_helper_; }

void SetFakeResponse(const std::string& url,
const std::string& data,
Expand Down Expand Up @@ -113,6 +122,9 @@ class AccountReconcilorTest : public testing::Test {
MockAccountReconcilor* mock_reconcilor_;
net::FakeURLFetcherFactory url_fetcher_factory_;
scoped_ptr<TestingProfileManager> testing_profile_manager_;
UMAHistogramHelper histogram_helper_;

DISALLOW_COPY_AND_ASSIGN(AccountReconcilorTest);
};

AccountReconcilorTest::AccountReconcilorTest()
Expand All @@ -122,8 +134,12 @@ AccountReconcilorTest::AccountReconcilorTest()
url_fetcher_factory_(NULL) {}

void AccountReconcilorTest::SetUp() {
CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableNewProfileManagement);
// If it's a non-parameterized test, or we have a parameter of true, set flag.
if (!::testing::UnitTest::GetInstance()->current_test_info()->value_param() ||
GetParam()) {
CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableNewProfileManagement);
}

testing_profile_manager_.reset(
new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
Expand Down Expand Up @@ -152,6 +168,10 @@ void AccountReconcilorTest::SetUp() {
token_service_ =
static_cast<FakeProfileOAuth2TokenService*>(
ProfileOAuth2TokenServiceFactory::GetForProfile(profile()));

// Take a new snapshot of the concerned histograms before each test
histogram_helper_.PrepareSnapshot(kHistogramsToSnapshot,
arraysize(kHistogramsToSnapshot));
}

MockAccountReconcilor* AccountReconcilorTest::GetMockReconcilor() {
Expand Down Expand Up @@ -339,7 +359,7 @@ TEST_F(AccountReconcilorTest, ValidateAccountsFromTokensFailedTokenRequest) {
ASSERT_EQ(1u, reconcilor->GetInvalidChromeAccountsForTesting().size());
}

TEST_F(AccountReconcilorTest, StartReconcileNoop) {
TEST_P(AccountReconcilorTest, StartReconcileNoop) {
signin_manager()->SetAuthenticatedUsername(kTestEmail);
token_service()->UpdateCredentials(kTestEmail, "refresh_token");

Expand Down Expand Up @@ -368,6 +388,12 @@ TEST_F(AccountReconcilorTest, StartReconcileNoop) {
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectTotalCount(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
}

// This is test is needed until chrome changes to use gaia obfuscated id.
Expand All @@ -378,7 +404,7 @@ TEST_F(AccountReconcilorTest, StartReconcileNoop) {
// tests makes sure that an email like "Dot.S@hmail.com", as seen by the
// token service, will be considered the same as "dots@gmail.com" as returned
// by gaia::ParseListAccountsData().
TEST_F(AccountReconcilorTest, StartReconcileNoopWithDots) {
TEST_P(AccountReconcilorTest, StartReconcileNoopWithDots) {
signin_manager()->SetAuthenticatedUsername("Dot.S@gmail.com");
token_service()->UpdateCredentials("Dot.S@gmail.com", "refresh_token");

Expand Down Expand Up @@ -409,9 +435,13 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopWithDots) {
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
}

TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
TEST_P(AccountReconcilorTest, StartReconcileNoopMultiple) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
Expand Down Expand Up @@ -448,9 +478,15 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectTotalCount(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
}

TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {
TEST_P(AccountReconcilorTest, StartReconcileAddToCookie) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
Expand All @@ -475,9 +511,106 @@ TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {
SimulateMergeSessionCompleted(reconcilor, "other@gmail.com",
GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.FirstRun", 1, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
}

TEST_P(AccountReconcilorTest, StartReconcileAddToCookieTwice) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");

EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("other@gmail.com"));
EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("third@gmail.com"));

SetFakeResponse(
GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK,
net::URLRequestStatus::SUCCESS);
SetFakeResponse(GaiaUrls::GetInstance()->people_get_url().spec(),
"{\"id\":\"foo\"}",
net::HTTP_OK,
net::URLRequestStatus::SUCCESS);

AccountReconcilor* reconcilor = GetMockReconcilor();
reconcilor->StartReconcile();
token_service()->IssueAllTokensForAccount(
"other@gmail.com",
"access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));
token_service()->IssueAllTokensForAccount(
"user@gmail.com",
"access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));

base::RunLoop().RunUntilIdle();
ASSERT_TRUE(reconcilor->is_reconcile_started_);
SimulateMergeSessionCompleted(
reconcilor, "other@gmail.com", GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.FirstRun", 1, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);

// Do another pass after I've added a third account to the token service

SetFakeResponse(
GaiaUrls::GetInstance()->list_accounts_url().spec(),
"[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1], "
"[\"b\", 0, \"n\", \"other@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
net::HTTP_OK,
net::URLRequestStatus::SUCCESS);
// This will cause the reconcilor to fire.
token_service()->UpdateCredentials("third@gmail.com", "refresh_token");

token_service()->IssueAllTokensForAccount(
"other@gmail.com",
"access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));
token_service()->IssueAllTokensForAccount(
"user@gmail.com",
"access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));
token_service()->IssueAllTokensForAccount(
"third@gmail.com",
"access_token",
base::Time::Now() + base::TimeDelta::FromHours(1));

base::RunLoop().RunUntilIdle();

ASSERT_TRUE(reconcilor->is_reconcile_started_);
SimulateMergeSessionCompleted(
reconcilor, "third@gmail.com", GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.FirstRun", 1, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.SubsequentRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.SubsequentRun", 1, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.SubsequentRun", 0, 1);
}

TEST_F(AccountReconcilorTest, StartReconcileAddToChrome) {
TEST_P(AccountReconcilorTest, StartReconcileAddToChrome) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");

Expand All @@ -500,9 +633,17 @@ TEST_F(AccountReconcilorTest, StartReconcileAddToChrome) {
ASSERT_TRUE(reconcilor->is_reconcile_started_);
SimulateRefreshTokenFetched(reconcilor, "other@gmail.com", "");
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.FirstRun", 0, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.FirstRun", 1, 1);
}

TEST_F(AccountReconcilorTest, StartReconcileBadPrimary) {
TEST_P(AccountReconcilorTest, StartReconcileBadPrimary) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
Expand Down Expand Up @@ -533,9 +674,17 @@ TEST_F(AccountReconcilorTest, StartReconcileBadPrimary) {
SimulateMergeSessionCompleted(reconcilor, "user@gmail.com",
GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);

histogram_helper()->Fetch();
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 1, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToCookieJar.FirstRun", 2, 1);
histogram_helper()->ExpectUniqueSample(
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
}

TEST_F(AccountReconcilorTest, StartReconcileOnlyOnce) {
TEST_P(AccountReconcilorTest, StartReconcileOnlyOnce) {
signin_manager()->SetAuthenticatedUsername(kTestEmail);
token_service()->UpdateCredentials(kTestEmail, "refresh_token");

Expand All @@ -560,7 +709,7 @@ TEST_F(AccountReconcilorTest, StartReconcileOnlyOnce) {
ASSERT_FALSE(reconcilor->is_reconcile_started_);
}

TEST_F(AccountReconcilorTest, StartReconcileWithSessionInfoExpiredDefault) {
TEST_P(AccountReconcilorTest, StartReconcileWithSessionInfoExpiredDefault) {
signin_manager()->SetAuthenticatedUsername("user@gmail.com");
token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
Expand Down Expand Up @@ -592,3 +741,7 @@ TEST_F(AccountReconcilorTest, StartReconcileWithSessionInfoExpiredDefault) {
GoogleServiceAuthError::AuthErrorNone());
ASSERT_FALSE(reconcilor->is_reconcile_started_);
}

INSTANTIATE_TEST_CASE_P(AccountReconcilorMaybeEnabled,
AccountReconcilorTest,
testing::Bool());
36 changes: 32 additions & 4 deletions chrome/test/base/uma_histogram_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,27 @@
#include "content/public/browser/histogram_fetcher.h"

UMAHistogramHelper::UMAHistogramHelper() {
base::StatisticsRecorder::Initialize();
}

UMAHistogramHelper::~UMAHistogramHelper() {
}

void UMAHistogramHelper::PrepareSnapshot(const char* const histogram_names[],
size_t num_histograms) {
for (size_t i = 0; i < num_histograms; ++i) {
std::string histogram_name = histogram_names[i];

base::HistogramBase* histogram =
base::StatisticsRecorder::FindHistogram(histogram_name);
// If there is no histogram present, then don't record a snapshot. The logic
// in the Expect* methods will act to treat no histogram equivalent to
// samples with zeros.
if (histogram) {
histogram_snapshots[histogram_name] =
make_linked_ptr(histogram->SnapshotSamples().release());
}
}
}

void UMAHistogramHelper::Fetch() {
Expand Down Expand Up @@ -82,18 +103,25 @@ void UMAHistogramHelper::CheckBucketCount(
base::HistogramBase::Sample sample,
base::HistogramBase::Count expected_count,
base::HistogramSamples& samples) {
EXPECT_EQ(expected_count, samples.GetCount(sample))
int actual_count = samples.GetCount(sample);
if (histogram_snapshots.count(name))
actual_count -= histogram_snapshots[name]->GetCount(sample);
EXPECT_EQ(expected_count, actual_count)
<< "Histogram \"" << name
<< "\" does not have the right number of samples (" << expected_count
<< ") in the expected bucket (" << sample << ").";
<< ") in the expected bucket (" << sample << "). It has (" << actual_count
<< ").";
}

void UMAHistogramHelper::CheckTotalCount(
const std::string& name,
base::HistogramBase::Count expected_count,
base::HistogramSamples& samples) {
EXPECT_EQ(expected_count, samples.TotalCount())
int actual_count = samples.TotalCount();
if (histogram_snapshots.count(name))
actual_count -= histogram_snapshots[name]->TotalCount();
EXPECT_EQ(expected_count, actual_count)
<< "Histogram \"" << name
<< "\" does not have the right total number of samples ("
<< expected_count << ").";
<< expected_count << "). It has (" << actual_count << ").";
}
Loading

0 comments on commit 3d08d1c

Please sign in to comment.