Create a histogram serialization mechanism in components/metrics

components/metrics/chromeos/metrics_utils_unittest.cc

+// 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.
+
+#include "components/metrics/chromeos/metrics_utils.h"
+
+#include "base/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "components/metrics/chromeos/metric_sample.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace metrics {
+namespace {
+
+class MetricsUtilsChromeOSTest : public testing::Test {
+ protected:
+  MetricsUtilsChromeOSTest() {
+    bool success = temporary_dir.CreateUniqueTempDir();
+    if (success) {
+      base::FilePath dir_path = temporary_dir.path();
+      filename = dir_path.value() + "chromeossampletest";
+      filepath = base::FilePath(filename);
+    }
+  }
+
+  virtual void SetUp() {

[Alexei Svitkine (slow), 2014/05/13 19:52:42]

OVERRIDE

[bsimonnet, 2014/05/13 21:28:00]

Done.

+    base::DeleteFile(filepath, false);
+  }
+
+  void TestSerialization(MetricSample* sample) {
+    std::string serialized(sample->ToString());
+    ASSERT_EQ('\0', serialized[serialized.length() - 1]);
+    EXPECT_TRUE(
+        sample->IsEqual(MetricsUtils::ReadSample(serialized).release()));

[Alexei Svitkine (slow), 2014/05/13 19:52:42]

I think you want .get() instead of .release() here, else you're leaking the
object.

[bsimonnet, 2014/05/13 21:28:00]

Done.

+  }
+
+  std::string filename;
+  base::ScopedTempDir temporary_dir;
+  base::FilePath filepath;
+};
+
+TEST_F(MetricsUtilsChromeOSTest, CrashSerializeTest) {
+  TestSerialization(MetricSample::CrashSample("test").release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, HistogramSerializeTest) {
+  TestSerialization(
+      MetricSample::HistogramSample("myhist", 13, 1, 100, 10).release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, LinearSerializeTest) {
+  TestSerialization(
+      MetricSample::LinearHistogramSample("linearhist", 12, 30).release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, SparseSerializeTest) {
+  TestSerialization(
+      MetricSample::SparseHistogramSample("mysparse", 30).release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, UserActionSerializeTest) {
+  TestSerialization(MetricSample::UserActionSample("myaction").release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, IllegalNameAreFilteredTest) {
+  scoped_ptr<MetricSample> sample1 =
+      MetricSample::SparseHistogramSample("no space", 10);
+  scoped_ptr<MetricSample> sample2 = MetricSample::LinearHistogramSample(
+      base::StringPrintf("here%cbhe", '\0'), 1, 3);
+
+  EXPECT_FALSE(MetricsUtils::WriteMetricToFile(sample1.get(), filename));
+  EXPECT_FALSE(MetricsUtils::WriteMetricToFile(sample2.get(), filename));
+  int64 size = 0;
+
+  ASSERT_TRUE(!PathExists(filepath) || base::GetFileSize(filepath, &size));
+
+  EXPECT_EQ(0, size);
+}
+
+TEST_F(MetricsUtilsChromeOSTest, BadInputIsCaughtTest) {
+  std::string input(
+      base::StringPrintf("sparsehistogram%cname foo%c", '\0', '\0'));
+  EXPECT_EQ(NULL, MetricSample::ReadSparseHistogram(input).release());
+}
+
+TEST_F(MetricsUtilsChromeOSTest, MessageSeparatedByZero) {
+  scoped_ptr<MetricSample> crash = MetricSample::CrashSample("mycrash");
+
+  MetricsUtils::WriteMetricToFile(crash.get(), filename);
+  int64 size = 0;
+  ASSERT_TRUE(base::GetFileSize(filepath, &size));
+  // 4 bytes for the size
+  // 5 bytes for crash
+  // 7 bytes for mycrash
+  // 2 bytes for the \0
+  // -> total of 18
+  EXPECT_EQ(size, 18);
+}
+
+TEST_F(MetricsUtilsChromeOSTest, MessagesTooLongAreDiscardedTest) {
+  // Creates a message that is bigger than the maximum allowed size.
+  // As we are adding extra character (crash, \0s, etc), if the name is
+  // kMessageMaxLength long, it will be too long.
+  std::string name(MetricsUtils::kMessageMaxLength, 'c');
+
+  scoped_ptr<MetricSample> crash = MetricSample::CrashSample(name);
+  EXPECT_FALSE(MetricsUtils::WriteMetricToFile(crash.get(), filename));
+  int64 size = 0;
+  ASSERT_TRUE(base::GetFileSize(filepath, &size));
+  EXPECT_EQ(0, size);
+}
+
+TEST_F(MetricsUtilsChromeOSTest, ReadLongMessageTest) {
+  base::File test_file(filepath,
+                       base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND);
+  std::string message(MetricsUtils::kMessageMaxLength + 1, 'c');
+
+  int32_t message_size = message.length() + sizeof(int32_t);

[Alexei Svitkine (slow), 2014/05/13 19:52:42]

Nit: Prefer int32 over int32_t types for new code. You seem to be using a mix of
the two throughout.

[bsimonnet, 2014/05/13 21:28:00]

Done.

+  test_file.WriteAtCurrentPos(reinterpret_cast<const char*>(&message_size),
+                              sizeof(message_size));
+  test_file.WriteAtCurrentPos(message.c_str(), message.length());
+  test_file.Close();
+
+  scoped_ptr<MetricSample> crash = MetricSample::CrashSample("test");
+  MetricsUtils::WriteMetricToFile(crash.get(), filename);
+
+  ScopedVector<MetricSample> samples;
+  MetricsUtils::ReadAndTruncateMetricsFromFile(filename, &samples);
+  ASSERT_EQ(size_t(1), samples.size());
+  EXPECT_TRUE(crash->IsEqual(samples[0]));
+}
+
+TEST_F(MetricsUtilsChromeOSTest, WriteReadTest) {
+  scoped_ptr<MetricSample> hist =
+      MetricSample::HistogramSample("myhist", 1, 2, 3, 4);
+  scoped_ptr<MetricSample> crash = MetricSample::CrashSample("mycrash");
+  scoped_ptr<MetricSample> lhist =
+      MetricSample::LinearHistogramSample("linear", 1, 10);
+  scoped_ptr<MetricSample> shist =
+      MetricSample::SparseHistogramSample("mysparse", 30);
+  scoped_ptr<MetricSample> action = MetricSample::UserActionSample("myaction");
+
+  MetricsUtils::WriteMetricToFile(hist.get(), filename);
+  MetricsUtils::WriteMetricToFile(crash.get(), filename);
+  MetricsUtils::WriteMetricToFile(lhist.get(), filename);
+  MetricsUtils::WriteMetricToFile(shist.get(), filename);
+  MetricsUtils::WriteMetricToFile(action.get(), filename);
+  ScopedVector<MetricSample> vect;
+  MetricsUtils::ReadAndTruncateMetricsFromFile(filename, &vect);
+  ASSERT_EQ(vect.size(), size_t(5));
+  EXPECT_TRUE(hist->IsEqual(vect[0]));
+  EXPECT_TRUE(crash->IsEqual(vect[1]));
+  EXPECT_TRUE(lhist->IsEqual(vect[2]));
+  EXPECT_TRUE(shist->IsEqual(vect[3]));
+  EXPECT_TRUE(action->IsEqual(vect[4]));
+
+  int64 size = 0;
+  ASSERT_TRUE(base::GetFileSize(filepath, &size));
+  ASSERT_EQ(0, size);
+}
+
+}  // namespace
+}  // namespace metrics