Add parser for new cpu freq file

chrome/browser/chromeos/power/cpu_data_collector_unittest.cc

+// Copyright 2017 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 <string>
+#include <vector>
+
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "chrome/browser/chromeos/power/cpu_data_collector.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace {
+// The suffix path of fake cpu frequency file for cpu0.
+const char time_in_state_suffix_path_cpu0[] = "cpu0/time_in_state";

[Daniel Erat, 2017/05/02 23:41:11]

please follow the style guide's naming convention for constants, e.g.
kTimeInStateSuffixPathCpu0.

https://google.github.io/styleguide/cppguide.html#Constant_Names

[weidongg, 2017/05/03 21:32:32]

Done.

+// The suffix path of fake cpu frequency file for cpu1.
+const char time_in_state_suffix_path_cpu1[] = "cpu1/time_in_state";
+// The suffix path of fake cpu frequency file for all cpus.
+const char all_time_in_state_suffix_path[] = "all_time_in_state";
+// The string content of the fake cpu frequency file for cpu0.
+const char time_in_state_content_cpu0[] =
+    "20000 30000000\n"
+    "60000 90000\n"
+    "100000 50000\n";
+// The string content of the fake cpu frequency file for cpu1.
+const char time_in_state_content_cpu1[] =
+    "20000 31000000\n"
+    "60000 91000\n"
+    "100000 51000\n";
+// The string content of the fake cpu frequency file for all cpus.
+const char all_time_in_state_content[] =
+    "freq\t\tcpu0\t\tcpu1\t\t\n"
+    "20000\t\t30000000\t\t31000000\t\t\n"
+    "60000\t\t90000\t\t91000\t\t\n"
+    "100000\t\t50000\t\t51000\t\t\n";
+// Expected cpu frequency state names after calling |ReadCpuFreqTimeInState| or
+// |ReadCpuFreqAllTimeInState|
+const std::vector<std::string> expected_cpu_freq_state_names = {"20", "60",

[Daniel Erat, 2017/05/02 23:41:10]

class-type static variables aren't allowed:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

use arrays instead.

[weidongg, 2017/05/03 21:32:32]

Done.

+                                                                "100"};
+// Expceted time_in_state of sample in |freq_samples| for cpu0.

[Daniel Erat, 2017/05/02 23:41:11]

"Expected"

[weidongg, 2017/05/03 21:32:32]

Done.
I put these expected variables in the test class and use std::vector so that
EXPECT_EQ can compare these with the results.

+const std::vector<int64_t> expected_time_in_state_cpu0{300000000, 900000,
+                                                       500000};
+// Expceted time_in_state of sample in |freq_samples| for cpu1.

[Daniel Erat, 2017/05/02 23:41:10]

|freq_samples| is a local var defined it tests; referring to it here doesn't
make sense

[weidongg, 2017/05/03 21:32:32]

Removed

+const std::vector<int64_t> expected_time_in_state_cpu1{310000000, 910000,
+                                                       510000};
+}  // namespace
+
+class CpuDataCollectorTest : public testing::Test {
+ public:
+  CpuDataCollectorTest() {}
+  ~CpuDataCollectorTest() override {}
+
+  void SetUp() override {
+    CHECK(temp_dir.CreateUniqueTempDir());

[Daniel Erat, 2017/05/02 23:41:10]

put code in c'tor and d'tor instead of SetUp/TearDown:
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

[weidongg, 2017/05/03 21:32:32]

Done.

+
+    time_in_state_path_cpu0 =
+        temp_dir.GetPath().AppendASCII(time_in_state_suffix_path_cpu0);
+    time_in_state_path_cpu1 =
+        temp_dir.GetPath().AppendASCII(time_in_state_suffix_path_cpu1);
+    all_time_in_state_path =
+        temp_dir.GetPath().AppendASCII(all_time_in_state_suffix_path);
+
+    CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu0));

[Daniel Erat, 2017/05/02 23:41:11]

remove all of these CreateTemporaryFile calls. see the function's documentation
-- they're actually creating new temp files, probably under /tmp, and returning
the paths. base::WriteFile will create new files at the paths you pass to it.

[weidongg, 2017/05/03 21:32:32]

When I removed these CreateTemporaryFile calls and run the test locally, the
following checks would fail. It seems that the file is not created by
base::WriteFile.

+    CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu1));
+    CHECK(base::CreateTemporaryFile(&all_time_in_state_path));
+    CHECK(base::WriteFile(
+              time_in_state_path_cpu0, time_in_state_content_cpu0,
+              static_cast<int>(strlen(time_in_state_content_cpu0))) != -1);
+    CHECK(base::WriteFile(
+              time_in_state_path_cpu1, time_in_state_content_cpu1,
+              static_cast<int>(strlen(time_in_state_content_cpu1))) != -1);
+    CHECK(base::WriteFile(
+              all_time_in_state_path, all_time_in_state_content,
+              static_cast<int>(strlen(all_time_in_state_content))) != -1);
+  }
+
+  void TearDown() override { CHECK(temp_dir.Delete()); }

[Daniel Erat, 2017/05/02 23:41:11]

you don't need this call. ScopedTempDir's d'tor already does this.

[weidongg, 2017/05/03 21:32:31]

Removed

+
+ protected:
+  base::ScopedTempDir temp_dir;

[Daniel Erat, 2017/05/02 23:41:10]

please read the style guide section on naming:
https://google.github.io/styleguide/cppguide.html#Variable_Names

[weidongg, 2017/05/03 21:32:32]

Done.

+  base::FilePath time_in_state_path_cpu0;

[Daniel Erat, 2017/05/02 23:41:11]

make all of these const after you initialize them in the c'tor

[weidongg, 2017/05/03 21:32:32]

The initialization of these variable should happen after
temp_dir_.CreateUniqueTempDir(), so it looks like they could not be marked
const. I put these variables in private, and mark the getter function const.

+  base::FilePath time_in_state_path_cpu1;
+  base::FilePath all_time_in_state_path;
+
+  DISALLOW_COPY_AND_ASSIGN(CpuDataCollectorTest);

[Daniel Erat, 2017/05/02 23:41:10]

put this in the private section

[weidongg, 2017/05/03 21:32:32]

Done.

+};
+
+TEST_F(CpuDataCollectorTest, ReadCpuFreqTimeInState) {
+  std::vector<std::string> cpu_freq_state_names;
+  CpuDataCollector::StateOccupancySample freq_sample_cpu0;
+  CpuDataCollector::StateOccupancySample freq_sample_cpu1;
+
+  CpuDataCollector::ReadCpuFreqTimeInState(time_in_state_path_cpu0.value(),
+                                           &cpu_freq_state_names,
+                                           &freq_sample_cpu0);
+  EXPECT_EQ(expected_cpu_freq_state_names, cpu_freq_state_names);
+  EXPECT_EQ(expected_time_in_state_cpu0, freq_sample_cpu0.time_in_state);
+
+  CpuDataCollector::ReadCpuFreqTimeInState(time_in_state_path_cpu1.value(),
+                                           &cpu_freq_state_names,
+                                           &freq_sample_cpu1);
+  EXPECT_EQ(expected_cpu_freq_state_names, cpu_freq_state_names);
+  EXPECT_EQ(expected_time_in_state_cpu1, freq_sample_cpu1.time_in_state);
+}
+
+TEST_F(CpuDataCollectorTest, ReadCpuFreqAllTimeInState) {
+  std::vector<std::string> cpu_freq_state_names;
+  std::vector<CpuDataCollector::StateOccupancySample> freq_samples;
+  CpuDataCollector::StateOccupancySample freq_sample_cpu0;
+  CpuDataCollector::StateOccupancySample freq_sample_cpu1;
+  // |ReadCpuFreqAllTimeInState| only completes sample for cpu that is online.
+  freq_sample_cpu0.cpu_online = true;
+  freq_sample_cpu1.cpu_online = true;
+  freq_samples.push_back(freq_sample_cpu0);
+  freq_samples.push_back(freq_sample_cpu1);
+
+  CpuDataCollector::ReadCpuFreqAllTimeInState(
+      2, all_time_in_state_path.value(), &cpu_freq_state_names, &freq_samples);
+  EXPECT_EQ(expected_cpu_freq_state_names, cpu_freq_state_names);
+  EXPECT_EQ(expected_time_in_state_cpu0, freq_samples[0].time_in_state);
+  EXPECT_EQ(expected_time_in_state_cpu1, freq_samples[1].time_in_state);
+}
+
+}  // namespace chromeos