Make profile statistics tasks inspectable by tests

chrome/browser/profiles/profile_statistics_browsertest.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stddef.h>
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/strings/stringprintf.h"
+#include "build/build_config.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/profiles/profile_statistics.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/test_password_store.h"
+#include "content/public/test/test_utils.h"
+
+namespace {
+
+const std::set<std::string> stats_categories {
+  profiles::kProfileStatisticsBrowsingHistory,
+  profiles::kProfileStatisticsPasswords,
+  profiles::kProfileStatisticsBookmarks,
+  profiles::kProfileStatisticsSettings};
+

[lwchkg, 2016/01/13 17:21:55]

Do you know why this empty row is marked as different?

[Mike Lerman, 2016/01/14 15:21:53]

It's been added; it doesn't exist on the left (e.g. there's no empty line there,
just the two const declarations)

[lwchkg, 2016/01/16 18:07:50]

Thanks. Didn't notice this one.

+const size_t num_of_stats_categories = stats_categories.size();
+
+bool IsProfileCategoryStatEqual(const profiles::ProfileCategoryStat& a,
+                                const profiles::ProfileCategoryStat& b) {
+  return a.category == b.category && a.count == b.count &&
+         a.success == b.success;
+}
+
+std::string ProfileCategoryStatToString(
+    const profiles::ProfileCategoryStat& a) {
+  return base::StringPrintf("category = %s, count = %d, success = %s",
+      a.category.c_str(), a.count, a.success ? "true" : "false");
+}
+
+::testing::AssertionResult AssertionProfileCategoryStatEqual(
+    const char* actual_expression,
+    const char* expected_expression,
+    const profiles::ProfileCategoryStat& actual_value,
+    const profiles::ProfileCategoryStat& expected_value) {
+  if (IsProfileCategoryStatEqual(actual_value, expected_value)) {
+    return ::testing::AssertionSuccess();
+  } else {
+    return ::testing::AssertionFailure()
+        << "Value of: " << actual_expression
+        << "\n  Actual: " << ProfileCategoryStatToString(actual_value)
+        << "\nExpected: " << expected_expression
+        << "\nWhich is: " << ProfileCategoryStatToString(expected_value);
+  }
+}
+
+::testing::AssertionResult AssertionProfileCategoryStatsEqual(
+    const char* actual_expression,
+    const char* expected_expression,
+    const profiles::ProfileCategoryStats& actual_value,

[Mike Lerman, 2016/01/14 15:21:53]

don't use "value", it conveys nothing.
actual_categories is better. (and expected_categories)

[lwchkg, 2016/01/16 18:07:50]

Acknowledged.

+    const profiles::ProfileCategoryStats& expected_value) {

[lwchkg, 2016/01/13 17:21:55]

Here's some sample output of the function.

e:\chromium\src\chrome\browser\profiles\profile_statistics_browsertest.cc(243):
error: ProfileCategoryStats are not equal.
  Actual: stats
Unmatched rows: category = Passwords, count = 0, success = false
                category = Settings, count = 26, success = true
                category = Testing, count = 5, success = true
Expected: profiles::GetProfileStatisticsFromCache(profile->GetPath())
Unmatched rows: category = Passwords, count = 0, success = true
                category = Settings, count = 23, success = true

[Mike Lerman, 2016/01/14 15:21:53]

Why only print the unmatched rows?? Why not just print out Everything about the
current state of each ProfileCategoryStats? I imagine, as someone debugging a
problem, that will be useful information to know. 

I'd either print the first thing that you noticed broke (hey, we're missing a
category!) or print out the whole state of the ProfileCategoryStats.

That should also make the function easier. You can just verify one aspect of the
equality at a time, and then report as necessary (all or just that piece of
informatoin).

[lwchkg, 2016/01/16 18:07:50]

I see. I'd print out the whole statistics since comparison cannot be made
simpler.

Anyway, I don't see either option can make the function easier. The statistics
may be added in a different order, so comparison must be done using an algorithm
like lines 68-83 below or the one in patch #1. (The algorithm is #1 is more
scalable, but that's not too important since we have only 4 statistics.)

For both algorithms, stopping at the first match failure doesn't make the
function much easier.

+  size_t actual_count = actual_value.size();
+  size_t expected_count = expected_value.size();
+  std::vector<bool> actual_unused(actual_count, true);
+  std::vector<bool> expected_unused(expected_count, true);
+
+  bool match_failed = actual_count != expected_count;

[Mike Lerman, 2016/01/14 15:21:53]

match of what? of counts. Perhaps call this counts_match (and invert the meaning
true/false)

[lwchkg, 2016/01/16 18:07:51]

Looks like I've named the variable incorrectly, so I'll rewrite to make it
clearer.

Anyway, the sets of statistics A and B are equal if and only if
(1) count of A = count of B, and also
(2) if a value exists in set A, it must also be exist in set B.

This line checks for (1).
(If the logic is not obvious to you, please tell because it means that I need to
add some comments to the source code.)

[anthonyvd, 2016/01/18 15:47:08]

You could probably just add a comment with the above in the function, it's super
clear when worded that way :)

[lwchkg, 2016/01/18 18:01:19]

Thanks. So comments will be added in the next patch.

[lwchkg, 2016/02/11 17:13:09]

Turns out std::is_permutation() was approved, so I can switch to that function
instead. Anyway std::is_permutation() should work exactly like the code here.

+  for (size_t actual_pos = 0; actual_pos < actual_count; actual_pos++) {
+    for (size_t expected_pos = 0; expected_pos < expected_count;
+         expected_pos++) {
+      if (expected_unused[expected_pos] &&
+          IsProfileCategoryStatEqual(actual_value[actual_pos],
+                                     expected_value[expected_pos])) {
+        actual_unused[actual_pos] = false;
+        expected_unused[expected_pos] = false;
+        break;
+      }
+    }
+
+    if (actual_unused[actual_pos])
+      match_failed = true;
+  }
+
+  if (!match_failed) {
+    return ::testing::AssertionSuccess();
+  } else {
+    ::testing::AssertionResult result = testing::AssertionFailure();
+    result << "ProfileCategoryStats are not equal.";
+
+    result << "\n  Actual: " << actual_expression;
+    bool actual_printed = false;
+    for (size_t i = 0; i < actual_count; i++) {
+      if (actual_unused[i]) {
+        result << "\n"
+               << (actual_printed ? "                " : "Unmatched rows: ")
+               << ProfileCategoryStatToString(actual_value[i]);
+        actual_printed = true;
+      }
+    }
+    if (!actual_printed)
+      result << "(no unmatched rows found in actual value)";
+
+
+    result << "\nExpected: " << expected_expression;
+    bool expected_printed = false;
+    for (size_t i = 0; i < expected_count; i++) {
+      if (expected_unused[i]) {
+        result << "\n"
+               << (expected_printed ? "                " : "Unmatched rows: ")
+               << ProfileCategoryStatToString(expected_value[i]);
+        expected_printed = true;
+      }
+    }
+    if (!expected_printed)
+      result << "(no unmatched rows found in expected value)";
+
+    return result;
+  }
+}
+
+class ProfileStatisticsAggregatorState {
+ public:
+  void callback(base::RunLoop* run_loop,
+                profiles::ProfileCategoryStats stats_return) {
+    size_t oldCount = stats_.size();
+    size_t newCount = stats_return.size();
+
+    EXPECT_LT(oldCount, newCount);
+    if (oldCount < newCount) {
+      for (size_t i = 0; i < oldCount; i++) {
+        // Exisiting statistics must be the same.
+        EXPECT_PRED_FORMAT2(AssertionProfileCategoryStatEqual,
+                            stats_[i], stats_return[i]);
+        // The new statistic categories must be different.
+        for (size_t j = oldCount; j < newCount; j++)
+          EXPECT_NE(stats_[i].category, stats_return[j].category);
+      }
+
+      for (size_t j = oldCount; j < newCount; j++) {
+        EXPECT_EQ(1u, stats_categories.count(stats_return[j].category));
+        // Count the number of statistics failures (incrementally).
+        if (!stats_return[j].success)
+          num_of_fails_++;
+      }
+      stats_ = stats_return;
+    }
+
+    EXPECT_GE(num_of_stats_categories, newCount);
+    if (num_of_stats_categories <= newCount)
+      run_loop->Quit();
+  }
+
+  profiles::ProfileCategoryStats GetStats() const { return stats_; }
+  int GetNumOfFails() const { return num_of_fails_; }
+
+ private:
+  profiles::ProfileCategoryStats stats_;
+  int num_of_fails_ = 0;
+};
+
+void WaitMilliseconds(int time_in_milliseconds) {
+  base::RunLoop run_loop;
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(),
+      base::TimeDelta::FromMilliseconds(time_in_milliseconds));
+  run_loop.Run();
+}
+
+}  // namespace
+
+class ProfileStatisticsBrowserTest : public InProcessBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    // Use TestPasswordStore to remove a possible race. Normally the
+    // PasswordStore does its database manipulation on the DB thread, which
+    // creates a possible race during navigation. Specifically the
+    // PasswordManager will ignore any forms in a page if the load from the
+    // PasswordStore has not completed.
+    PasswordStoreFactory::GetInstance()->SetTestingFactory(
+        browser()->profile(),
+        password_manager::BuildPasswordStore<
+            content::BrowserContext, password_manager::TestPasswordStore>);
+  }
+};
+
+using ProfileStatisticsBrowserDeathTest = ProfileStatisticsBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(ProfileStatisticsBrowserTest, GatherStatistics) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  ASSERT_TRUE(profile);
+
+  ProfileStatisticsAggregatorState state;
+
+  base::RunLoop run_loop;
+  profiles::GatherProfileStatistics(
+      profile,
+      base::Bind(&ProfileStatisticsAggregatorState::callback,
+                 base::Unretained(&state), &run_loop),
+      nullptr);
+  run_loop.Run();
+
+  EXPECT_EQ(0, state.GetNumOfFails());
+
+  profiles::ProfileCategoryStats stats = state.GetStats();
+  for (const auto& stat : stats) {
+    if (stat.category != profiles::kProfileStatisticsSettings)
+      EXPECT_EQ(0, stat.count);
+  }
+
+  EXPECT_PRED_FORMAT2(AssertionProfileCategoryStatsEqual, stats,
+      profiles::GetProfileStatisticsFromCache(profile->GetPath()));
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileStatisticsBrowserTest, CloseBrowser) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  ASSERT_TRUE(profile);
+
+  CloseBrowserSynchronously(browser());
+
+  profiles::ProfileCategoryStats stats;
+  // Wait for at most 2 seconds for the statistics to be gathered.

[anthonyvd, 2016/01/18 15:47:08]

This looks to me like it has the potential to be flaky. Is there a different way
to do it? Is there a way to observe this process/be notified when the stats are
done being gathered?

[lwchkg, 2016/01/18 18:01:19]

The same topic is still being discussed in patch #1, so let's continue there.

Anyway, here's the summary: I'll modify profile_statistics.cc to allow
inspecting from the outside.

+  for (int i = 0; i < 10; i++) {
+    WaitMilliseconds(200);
+    stats = profiles::GetProfileStatisticsFromCache(profile->GetPath());
+    EXPECT_EQ(num_of_stats_categories, stats.size());
+
+    bool allSucceed = true;
+    for (const auto& stat : stats)
+      allSucceed &= stat.success;
+    if (allSucceed)
+      break;
+  }
+
+  for (const auto& stat : stats) {
+    EXPECT_EQ(1u, stats_categories.count(stat.category));
+    EXPECT_TRUE(stat.success);
+    if (stat.category != profiles::kProfileStatisticsSettings)
+      EXPECT_EQ(0, stat.count);
+  }
+  // Wait for background tasks to complete, otherwise some warning about
+  // the failure to post a task will be displayed.
+  WaitMilliseconds(500);
+}