Revert 149766 - Performance monitor stats gathering.

chrome/browser/performance_monitor/performance_monitor_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 "chrome/test/base/in_process_browser_test.h"
 
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/performance_monitor/constants.h"
 #include "chrome/browser/performance_monitor/database.h"
 #include "chrome/browser/performance_monitor/performance_monitor.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 
 using extensions::Extension;
 using performance_monitor::Event;
 
 namespace {
-
 // Helper struct to store the information of an extension; this is needed if the
 // pointer to the extension ever becomes invalid (e.g., if we uninstall the
 // extension).
 struct ExtensionBasicInfo {
   // Empty constructor for stl-container-happiness.
   ExtensionBasicInfo() {
   }
   explicit ExtensionBasicInfo(const Extension* extension)
       : description(extension->description()),
         id(extension->id()),
@@ skipping 90 matching lines @@
     // to complete fully before proceeding with the test.
     content::WindowedNotificationObserver windowed_observer(
         chrome::NOTIFICATION_PERFORMANCE_MONITOR_INITIALIZED,
         content::NotificationService::AllSources());
 
     performance_monitor_->Start();
 
     windowed_observer.Wait();
   }
 
-  // A handle for gathering statistics from the database, which must be done on
-  // the background thread. Since we are testing, we can mock synchronicity with
-  // FlushForTesting().
-  void GatherStatistics() {
-    content::BrowserThread::PostBlockingPoolSequencedTask(
-        Database::kDatabaseSequenceToken,
-        FROM_HERE,
-        base::Bind(&PerformanceMonitor::GatherStatisticsOnBackgroundThread,
-                   base::Unretained(performance_monitor())));
-
-    content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  }
-
   void GetEventsOnBackgroundThread(std::vector<linked_ptr<Event> >* events) {
     // base::Time is potentially flaky in that there is no guarantee that it
     // won't actually decrease between successive calls. If we call GetEvents
     // and the Database uses base::Time::Now() and gets a lesser time, then it
     // will return 0 events. Thus, we use a time that is guaranteed to be in the
     // future (for at least the next couple hundred thousand years).
     *events = performance_monitor_->database()->GetEvents(
         base::Time(), base::Time::FromInternalValue(kint64max));
   }
 
@@ skipping 11 matching lines @@
         Database::kDatabaseSequenceToken,
         FROM_HERE,
         base::Bind(&PerformanceMonitorBrowserTest::GetEventsOnBackgroundThread,
                    base::Unretained(this),
                    &events));
 
     content::BrowserThread::GetBlockingPool()->FlushForTesting();
     return events;
   }
 
-  void GetStatsOnBackgroundThread(Database::MetricInfoVector* metrics,
-                                  MetricType type) {
-    *metrics = performance_monitor_->database()->GetStatsForActivityAndMetric(
-        type, base::Time(), base::Time::FromInternalValue(kint64max));
-  }
-
-  // A handle for getting statistics from the database (see previous comments on
-  // GetEvents() and GetEventsOnBackgroundThread).
-  Database::MetricInfoVector GetStats(MetricType type) {
-    content::BrowserThread::GetBlockingPool()->FlushForTesting();
-    content::RunAllPendingInMessageLoop();
-
-    Database::MetricInfoVector metrics;
-    content::BrowserThread::PostBlockingPoolSequencedTask(
-        Database::kDatabaseSequenceToken,
-        FROM_HERE,
-        base::Bind(&PerformanceMonitorBrowserTest::GetStatsOnBackgroundThread,
-                   base::Unretained(this),
-                   &metrics,
-                   type));
-
-    content::BrowserThread::GetBlockingPool()->FlushForTesting();
-    return metrics;
-  }
-
   // A handle for inserting a state value into the database, which must be done
   // on the background thread. This is useful for mocking up a scenario in which
   // the database has prior data stored. We mock synchronicity with
   // FlushForTesting().
   void AddStateValue(const std::string& key, const std::string& value) {
     content::BrowserThread::PostBlockingPoolSequencedTask(
         Database::kDatabaseSequenceToken,
         FROM_HERE,
         base::Bind(base::IgnoreResult(&Database::AddStateValue),
                    base::Unretained(performance_monitor()->database()),
@@ skipping 263 matching lines @@
 
   std::string previous_version;
   std::string current_version;
 
   ASSERT_TRUE(value->GetString("previousVersion", &previous_version));
   ASSERT_EQ(kOldVersion, previous_version);
   ASSERT_TRUE(value->GetString("currentVersion", &current_version));
   ASSERT_EQ(version_string, current_version);
 }
 
-IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, GatherStatistics) {
-  GatherStatistics();
-
-  // Gather CPU usage. No stats should be recorded because this was the first
-  // call to GatherStatistics.
-  Database::MetricInfoVector stats = GetStats(METRIC_CPU_USAGE);
-  ASSERT_EQ(0u, stats.size());
-
-  // Gather private memory usage.
-  stats = GetStats(METRIC_PRIVATE_MEMORY_USAGE);
-  ASSERT_EQ(1u, stats.size());
-  EXPECT_GT(stats[0].value, 0);
-
-  // Gather shared memory usage.
-  stats = GetStats(METRIC_SHARED_MEMORY_USAGE);
-  ASSERT_EQ(1u, stats.size());
-  EXPECT_GT(stats[0].value, 0);
-
-  // Spin for a while, so CPU usage isn't 0.
-  const int kSpinCount = 1000000000;
-  int i = 0;
-  for (; i < kSpinCount; ++i) {
-  }
-  ASSERT_EQ(kSpinCount, i);
-
-  GatherStatistics();
-
-  // Gather CPU usage a second time and verify a stat was recorded.
-  stats = GetStats(METRIC_CPU_USAGE);
-  ASSERT_EQ(1u, stats.size());
-  EXPECT_GT(stats[0].value, 0);

[mitchellwrosen, 2012/08/07 17:26:32]

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
404'd, is there an alternate URL I can look at?

Anyways, I believe it was this EXPECT_GT that failed. The whole spin-for-a-while
is a bit of a hack: all I'm trying to do is cause the process to use a
noticeable percentage of CPU cycles so that I can verify a number > 0 was
inserted as a statistic. Seems more like a test for ProcessMetrics::GetCPUUsage,
and relies on my dumb for loop working as intended.

Would an appropriate fix be to delete the entire test from the for-loop spin and
down?

[Yoyo Zhou, 2012/08/07 19:09:50]

Your assessment looks right:

http://chromium-build-logs.appspot.com/gtest_query?gtest_query=PerformanceMon...

It seems like you could just remove the spinloop and the EXPECT_GT checks. This
way you would still test that a second value was recorded. But it's up to you
whether you think this is worth testing.

-
-  // Gather private memory usage a second time and verify a second stat was
-  // recorded.
-  stats = GetStats(METRIC_PRIVATE_MEMORY_USAGE);
-  ASSERT_EQ(2u, stats.size());
-  EXPECT_GT(stats[1].value, 0);
-
-  // Gather shared memory usage a second time and verify a second stat was
-  // recorded.
-  stats = GetStats(METRIC_SHARED_MEMORY_USAGE);
-  ASSERT_EQ(2u, stats.size());
-  EXPECT_GT(stats[1].value, 0);
-}
-
 #if !defined(OS_WIN)
 // Disabled on Windows due to a bug where Windows will return a normal exit
 // code in the testing environment, even if the process died (this is not the
 // case when hand-testing). This code can be traced to MSDN functions in
 // base::GetTerminationStatus(), so there's not much we can do.
 IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, KilledByOSEvent) {
   content::CrashTab(chrome::GetActiveWebContents(browser()));
 
   std::vector<linked_ptr<Event> > events = GetEvents();
 
@@ skipping 69 matching lines @@
 
   std::string event_profile;
   ASSERT_TRUE(events[0]->data()->GetString("profileName", &event_profile));
   ASSERT_EQ(first_profile_name_, event_profile);
 
   ASSERT_TRUE(events[1]->data()->GetString("profileName", &event_profile));
   ASSERT_EQ(second_profile_name_, event_profile);
 }
 
 }  // namespace performance_monitor