Add ExitFunnel to prepare for instrumenting browser exits.

components/browser_watcher/watcher_metrics_provider_win_unittest.cc

 // Copyright (c) 2014 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 "components/browser_watcher/watcher_metrics_provider_win.h"
 
 #include <cstdlib>
 
 #include "base/process/process_handle.h"
 #include "base/strings/string16.h"
@@ skipping 38 matching lines @@
     // Write the exit code to registry.
     ULONG result = key.WriteValue(key_name.c_str(), exit_code);
     ASSERT_EQ(result, ERROR_SUCCESS);
   }
 
   size_t ExitCodeRegistryPathValueCount() {
     base::win::RegKey key(HKEY_CURRENT_USER, kRegistryPath, KEY_READ);
     return key.GetValueCount();
   }
 
+  void AddExitFunnelEvent(int pid, const base::char16* name, int64 value) {
+    base::string16 key_name =
+        base::StringPrintf(L"%ls\\%d-%d", kRegistryPath, pid, pid);

[erikwright (departed), 2014/12/11 15:00:46]

Why dup the writing? You're testing (and overly coupling your tests to)
implementation.

You should have a reader and a writer. And the public contract of the two is
that what I write with the Writer can be read from the Reader. Your tests
shouldn't concern themselves with how that data is stored under the hood.

[Sigurður Ásgeirsson, 2014/12/12 16:21:40]

There's really not enough code there to worry about reuse. Also, making the
writing reusable for testing means to allow specifying the time written.

+
+    base::win::RegKey key(HKEY_CURRENT_USER, key_name.c_str(), KEY_WRITE);
+    ASSERT_EQ(key.WriteValue(name, &value, sizeof(value), REG_QWORD),
+              ERROR_SUCCESS);
+  }
+
  protected:
   registry_util::RegistryOverrideManager override_manager_;
   base::HistogramTester histogram_tester_;
 };
 
 }  // namespace
 
 TEST_F(WatcherMetricsProviderWinTest, RecordsStabilityHistogram) {
   // Record multiple success exits.
   for (size_t i = 0; i < 11; ++i)
@@ skipping 27 matching lines @@
   WatcherMetricsProviderWin provider(kRegistryPath);
 
   provider.ProvideStabilityMetrics(NULL);
   histogram_tester_.ExpectUniqueSample(
         WatcherMetricsProviderWin::kBrowserExitCodeHistogramName, 0, 11);
 
   // Verify that the reported values are gone.
   EXPECT_EQ(ExitCodeRegistryPathValueCount(), 1);
 }
 
+TEST_F(WatcherMetricsProviderWinTest, RecordsExitFunnel) {
+  // Record an exit funnel - note the recorded times are in microseconds,
+  // but the reporting is in milliseconds, hence the times 1000.
+  AddExitFunnelEvent(100, L"One", 1000 * 1000);
+  AddExitFunnelEvent(100, L"Two", 1010 * 1000);
+  AddExitFunnelEvent(100, L"Three", 990 * 1000);
+
+  WatcherMetricsProviderWin provider(kRegistryPath);
+
+  provider.ProvideStabilityMetrics(NULL);

[erikwright (departed), 2014/12/11 15:00:46]

Testing that the histograms are actually written is good, but it's going to be
inconvenient to get really thorough coverage of your reading code if that's the
only way to do it.

Take the reading code out of the WatcherMetricsProvider, test the hell out of
it, and then you can easily get high coverage of the WatcherMetricsProvider by
making the simple verification that it does in fact send histograms.

[Sigurður Ásgeirsson, 2014/12/12 16:21:40]

I added some testing to verify that the metrics provider can read what the exit
funnel writes. I'd like to ship this as-is, rather than sinking further work
into refining it. This code won't live long, and we only need confidence that
it's not reporting rubbish.

+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.Three", 0, 1);
+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.One", 10, 1);
+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.Two", 20, 1);
+
+  // Make sure the subkey is deleted on reporting.
+  base::win::RegistryKeyIterator it(HKEY_CURRENT_USER, kRegistryPath);
+  ASSERT_EQ(it.SubkeyCount(), 0);
+}
+
 }  // namespace browser_watcher