Make profile statistics tasks inspectable by tests

chrome/browser/profiles/profile_statistics_browsertest.cc

Index: chrome/browser/profiles/profile_statistics_browsertest.cc
diff --git a/chrome/browser/profiles/profile_statistics_browsertest.cc b/chrome/browser/profiles/profile_statistics_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e6db58232bf86aab68e4454ef223914c38de52b6
--- /dev/null
+++ b/chrome/browser/profiles/profile_statistics_browsertest.cc
@@ -0,0 +1,186 @@
+// 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 <algorithm>
+
+#include "base/bind.h"
+#include "base/macros.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 profiles {
+  bool operator==(const ProfileCategoryStat& a,
+                  const ProfileCategoryStat& b) {
+    return a.category == b.category && a.count == b.count &&
+           a.success == b.success;
+  }
+
+  bool operator<(const ProfileCategoryStat& a, const ProfileCategoryStat& b) {

[Mike Lerman, 2016/01/11 18:12:20]

it would be kinda nice if we didn't need these operators.

[lwchkg, 2016/01/13 12:58:47]

Didn't recognize that the need to put operators/functions in ::profiles is a
deal-breaker. (If two files define the same operator/function the linker will
fail.) I was simply too amused with Google test's features. Looks like I still
have lots to learn.

So I'll turn ::profiles::*, operator or not, into something else.

+    return std::tie(a.category, a.count, a.success) <
+           std::tie(b.category, b.count, b.success);
+  }
+
+  void PrintTo(const ProfileCategoryStat& a, ::std::ostream* os) {

[Mike Lerman, 2016/01/11 18:12:20]

What is this for?

[lwchkg, 2016/01/13 12:58:47]

It would be nice if we can print out a whole structure (category, count,
success) if any part of it is different.

BTW, I'll try to achieve this in another way without polluting ::profiles.

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

I'd conversely prefer to test each individual part (whenever we want to verify
that each part IS indeed correct); that way, the assertion will automatically
just tell us what went wrong.

+    *os << "category = " << a.category
+        << ", count = " << a.count
+        << ", success = " << (a.success ? "true" : "false");
+  }
+}  // namespace profiles
+
+namespace {
+
+const std::set<std::string> stats_categories {
+  profiles::kProfileStatisticsBrowsingHistory,
+  profiles::kProfileStatisticsPasswords,
+  profiles::kProfileStatisticsBookmarks,
+  profiles::kProfileStatisticsSettings};
+const size_t num_of_stats_categories = stats_categories.size();
+
+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_EQ(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();
+}
+
+// Check if the statistics are the same. Since the statistics can be given in
+// any order, the entries are sorted before comparison.
+void ExpectProfileStatisticsEqual(profiles::ProfileCategoryStats actual_stats,

[Mike Lerman, 2016/01/11 18:12:20]

these should be ref's (&)

[lwchkg, 2016/01/13 12:58:47]

I sort the statistics before comparison, and doing so modifies the original
content of the arguments. As the user does not expect a modified
ProfileCategoryStatistics by calling this function, so I played safe and copied
the contents of both stats objects.

Anyway, if you're not into the sorting implementation, I can use std::find
instead to match the results, that is less scalable, but it shouldn't matter
since there are just a few statistics anyway.

+    profiles::ProfileCategoryStats expected_stats) {
+  size_t expected_count = expected_stats.size();
+  size_t actual_count = actual_stats.size();
+  EXPECT_EQ(expected_count, actual_count);
+  if (expected_count == actual_count) {
+    std::sort(expected_stats.begin(), expected_stats.end());
+    std::sort(actual_stats.begin(), actual_stats.end());
+
+    for (size_t i = 0; i < expected_count; i++)
+      EXPECT_EQ(expected_stats[i], actual_stats[i]);

[Mike Lerman, 2016/01/11 18:12:20]

Can we compare the members of ProfileCategoryStats individually rather than
needing to define operator== above? This will additionally let us determine what
aspect of the objects aren't the same.

[lwchkg, 2016/01/13 12:58:47]

Actually I want to compare the whole thing, and print the whole stat out if
there is any difference. So readers will be able to see what exactly is wrong
about the statistics in the log, instead of only knowing the count of an unknown
stat should be 24 instead of 23.

BTW, I'll replace those namespace cursed operator== and PrintTo by predicate
formatter assertions. Maybe replace this function too so readers will know which
line did the comparison fail, but it will be a little bit complex.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

+  }
+}
+
+}  // 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);
+  }
+
+  ExpectProfileStatisticsEqual(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.
+  for (int i = 0; i < 10; i++) {
+    WaitMilliseconds(200);

[Mike Lerman, 2016/01/11 18:12:20]

We really really should properly wait for the statistics to complete via
Callback. This will almost certainly be flaky.

[lwchkg, 2016/01/13 12:58:47]

I'd like to use a callback, but it is not possible as the writes of
ProfileInfoCache is triggered by CloseBrowserSynchronously, for which I cannot
inject a callback into.

Anyway, RunLoop::RunUntilIdle is also not a solution because statistics
callbacks may fire after RunUntilIdle return.

If you have any brilliant ideas injecting a good callback then I'd like to know.

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

What are we really trying to test here? We're trying to test that closing the
Browser (specifically, the last browser, which is a more interesting test, so
try opening multiple) causes the ProfileStatistics to start to be gathered. We
really don't need to verify the accuracy of that process in this test - that's
for another unit test.

Which means we probably need a way to know if we have a current executing
ProfileStatisticsAggregator running for a Profile. This makes sense anyways - we
probably don't want to be executing multiple ProfileStatisticsAggregators at the
same time for a Profile; we should cancel the currently executing one (if
possible?) and create a new one. This implies there SHOULD be some way to track
this, and then verify that in this test.

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

Interesting. I have a question though: if we execute two
ProfileStatisticsAggregators with the same Profile but with a different
callback, what should be the result? Should I call both callbacks when the
statistics arrive?

+    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);
+}