Improve UMA tests; improve no-op reconcilor; new reconcilor test.

chrome/test/base/uma_histogram_helper.h

 // 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.
 
 #ifndef CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
 #define CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_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 {
  public:
+  // UMAHistogramHelper should be created before the execution of the test case.
   UMAHistogramHelper();
 
+  ~UMAHistogramHelper();
+
+  // Parameters should be string literals of all histograms to snapshot.
+  // Call this before executing the test code.
+  void PrepareSnapshot(const char* histogram_names[], int num_histograms);

[Ilya Sherman, 2014/06/13 19:58:12]

nit: "int" -> "size_t"

[Ilya Sherman, 2014/06/13 19:58:12]

Actually, is this function needed at all?  Why not just snapshot all histograms
at the time this class is created, as the StatisticsDeltaReader does?

[Ilya Sherman, 2014/06/13 19:58:12]

nit: "const char*" -> "const char* const"

[Mike Lerman, 2014/06/16 14:59:13]

Done.

[Mike Lerman, 2014/06/16 14:59:14]

Efficiency's sake? There can be dozens if not hundreds of histograms in the UMA
system but we likely only care about a small number. I assume that only taking
snapshots of a few is faster, and speed of unit test execution is a good thing.

[Mike Lerman, 2014/06/16 14:59:14]

Done.

[Ilya Sherman, 2014/06/16 18:11:29]

I'm not convinced that the small bit of efficiency is worthwhile, given that
this requires test authors to have to add a bunch of boilerplate to their test
code.  Unless you measure the performance delta, and find that there is a
significant difference, I'd prefer to go with the option that's simpler for test
authors.  But, I don't feel super strongly about this.

+
   // 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();
 
   // We know the exact number of samples in a bucket, and that no other bucket
-  // should have samples.
+  // should have samples. If |PrepareSnapshot| was called for |name| histogram,

[Ilya Sherman, 2014/06/13 19:58:12]

nit: "for |name| histogram" -> "for the histogram named |name|" or "for the
histogram with the given |name|" or something like that

[Mike Lerman, 2014/06/16 14:59:14]

Done.

+  // then the |expected_count| is the diff from the snapshot.

[Ilya Sherman, 2014/06/13 19:58:12]

Is it worth documenting what happens if PrepareSnapshot was called multiple
times?

[Mike Lerman, 2014/06/16 14:59:14]

Yes. I wrote wordy text, but would almost rather right something like "Expect
Map mechanics using the histogram's name as a key and the histogram's snapshot
as the value," as this should be clear to most programmers.

   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.
+  // have samples as well. If |PrepareSnapshot| was called for |name| histogram,
+  // 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);
 
-  // We don't know the values of the samples, but we know how many there are.
+  // 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);
 
  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);
 
   DISALLOW_COPY_AND_ASSIGN(UMAHistogramHelper);
+
+  // The map from histogram names to their snapshots
+  std::map<std::string, scoped_ptr<base::HistogramSamples> >

[Ilya Sherman, 2014/06/13 19:58:12]

Hmm, I'm like 99% sure that scoped_ptrs aren't safe to store in std::map<>'s.  I
think you want either a linked_ptr<> or to just follow the same pattern used in
the StatisticsDeltaReader class.

[Mike Lerman, 2014/06/16 14:59:14]

Good to know. I'll go with a linked_ptr. Since I'm explicitly supporting
multiple calls, and for a unit test I'd rather delegate managing the memory
references to the pointer class.

+      histogram_snapshots;
 };
 
 #endif  // CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_