Rework UMAHistogramHelper and StatisticsDeltaReader into [Chrome]HistogramTester.

base/test/histogram_tester.h

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 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.
 
-#ifndef CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
-#define CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
+#ifndef BASE_TEST_HISTOGRAM_TESTER_H_
+#define BASE_TEST_HISTOGRAM_TESTER_H_
 
-#include "base/memory/linked_ptr.h"
+#include <map>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
-#include "base/metrics/histogram_samples.h"
 
-// UMAHistogramHelper provides a simple interface for examining UMA histograms.
-// Tests can use this interface to verify that UMA data is getting logged as
-// intended.
-class UMAHistogramHelper {
+namespace base {
+
+class HistogramSamples;
+
+// HistogramTester provides a simple interface for examining histograms, UMA
+// or otherwise. Tests can use this interface to verify that Histograms data is

[Ilya Sherman, 2014/07/15 03:56:34]

nit: "Histograms" -> "histograms"

[Mike Lerman, 2014/07/16 17:29:02]

Done.

+// getting logged as intended.
+// Note that if the test lies within /chrome, please use the
+// |ChromeHistogramTester| class.

[Ilya Sherman, 2014/07/15 03:56:34]

nit: Please omit this sentence -- it's actually better to use the simpler class
where feasible, IMO; and it's also awkward to refer to a higher-level source
directory from a lower-level one.

[Mike Lerman, 2014/07/16 17:29:02]

Done.

+class HistogramTester {
  public:
-  // UMAHistogramHelper should be created before the execution of the test case.
-  UMAHistogramHelper();
+  // The constructor will call StatisticsRecorder::Initialize() for you. Also,
+  // this takes a snapshot of all current histograms counts.
+  HistogramTester();
+  ~HistogramTester();
 
-  ~UMAHistogramHelper();
-
-  // Parameters should be string literals of all histograms to snapshot.
-  // Call this before executing the test code. This method can be called
-  // multiple times. The existing snapshots are preserved, except when one of
-  // the |histogram_names| was previously passed as a parameter, then a new
-  // snapshot will replace the existing one.
-  void PrepareSnapshot(const char* const histogram_names[],
-                       size_t num_histograms);
-
-  // Each child process may have its own histogram data, make sure this data
-  // gets accumulated into the browser process before we examine the histograms.
-  void Fetch();
+  // Fetch the values to be tested when the test uses multiple threads. Call
+  // this after the test code has been executed but before performing the
+  // necessary assertions. This may be called multiple times.
+  virtual void FetchTestingSnapshot();

[Ilya Sherman, 2014/07/15 03:56:35]

It looks like this method can be omitted from the base class, and just defined
in the //chrome subclass.

[Mike Lerman, 2014/07/16 17:29:03]

Done.

 
   // We know the exact number of samples in a bucket, and that no other bucket
   // should have samples. If |PrepareSnapshot| was called for the histogram
   // named |name| then the |expected_count| is the diff from the snapshot.

[Ilya Sherman, 2014/07/15 03:56:34]

Please update this comment -- PrepareSnapshot() is no longer a method in this
class.  Ditto for the other comments.

[Mike Lerman, 2014/07/16 17:29:02]

Done.

   void ExpectUniqueSample(const std::string& name,
                           base::HistogramBase::Sample sample,
                           base::HistogramBase::Count expected_count);
 
   // We know the exact number of samples in a bucket, but other buckets may
   // have samples as well. If |PrepareSnapshot| was called for histogram named
   // |name| then the |expected_count| is the diff from the snapshot.
   void ExpectBucketCount(const std::string& name,
                           base::HistogramBase::Sample sample,
                           base::HistogramBase::Count expected_count);

[Ilya Sherman, 2014/07/15 03:56:35]

nit: Please fix up the alignment of these two lines.

[Mike Lerman, 2014/07/16 17:29:03]

Done.

 
   // We don't know the values of the samples, but we know how many there are. If
   // |PrepareSnapshot| was called for |name| histogram, then the
   // |count| is the diff from the snapshot.
   void ExpectTotalCount(const std::string& name,
                         base::HistogramBase::Count count);
 
+  // Directly access the base snapshot taken for a particular histogram.
+  scoped_ptr<HistogramSamples> GetBaseHistogramSamples(

[Ilya Sherman, 2014/07/15 03:56:35]

nit: What does the word "base" mean in this context?

[Mike Lerman, 2014/07/16 17:29:03]

Nothing useful. Name and comment changed.

+      const std::string& histogram_name);

[Ilya Sherman, 2014/07/15 03:56:34]

Is this method still useful, or can all clients of this class be served by the
three ExpectFoo() methods defined above?

[Mike Lerman, 2014/07/16 17:29:02]

I had hoped so, but there is at least one test that does other math using the
snapshot's value, so I have to still expose this.

+
  private:
-  void FetchCallback();
-
   void CheckBucketCount(const std::string& name,
                         base::HistogramBase::Sample sample,
                         base::Histogram::Count expected_count,
                         base::HistogramSamples& samples);
 
   void CheckTotalCount(const std::string& name,
                        base::Histogram::Count expected_count,
                        base::HistogramSamples& samples);

[Ilya Sherman, 2014/07/15 03:56:35]

Please document these two methods.

[Mike Lerman, 2014/07/16 17:29:02]

Done.

 
-  DISALLOW_COPY_AND_ASSIGN(UMAHistogramHelper);
+  // Used to determine the histogram changes made during this instance's
+  // lifecycle. This instance takes ownership of the samples, which are deleted
+  // when the instance is destroyed.
+  std::map<std::string, HistogramSamples*> base_snapshot_;

[Ilya Sherman, 2014/07/15 03:56:34]

nit: WDYT of "initial_snapshot_" rather than "base_snapshot_"?

[Mike Lerman, 2014/07/16 17:29:02]

There's only one snapshot, so it doesn't really need a prefix at all that
relates to timing. If any prefix is supplied, it should describe that this
contains data of all histogram samples. So perhaps histograms_snapshot_?

 
-  // The map from histogram names to their snapshots
-  std::map<std::string, linked_ptr<base::HistogramSamples> >
-      histogram_snapshots;
+  DISALLOW_COPY_AND_ASSIGN(HistogramTester);
 };
 
-#endif  // CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
+}  // namespace base
+
+#endif  // BASE_TEST_HISTOGRAM_TESTER_H_