Add parser for new cpu freq file

chrome/browser/chromeos/power/cpu_data_collector_unittest.cc

Index: chrome/browser/chromeos/power/cpu_data_collector_unittest.cc
diff --git a/chrome/browser/chromeos/power/cpu_data_collector_unittest.cc b/chrome/browser/chromeos/power/cpu_data_collector_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..29bb1d5cbe422a8fae36bfbaba20ebcdbf315e97
--- /dev/null
+++ b/chrome/browser/chromeos/power/cpu_data_collector_unittest.cc
@@ -0,0 +1,137 @@
+// 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"
+
+using base::TimeDelta;
+
+namespace chromeos {
+namespace {
+
+// The suffix path of fake cpu frequency file for cpu0.
+const char kTimeInStateSuffixPathCpu0[] = "cpu0/time_in_state";

[xiyuan, 2017/05/04 20:12:01]

nit: const -> constexpr, here and other places.

[weidongg, 2017/05/04 20:32:46]

Done.

+
+// The suffix path of fake cpu frequency file for cpu1.
+const char kTimeInStateSuffixPathCpu1[] = "cpu1/time_in_state";
+
+// The suffix path of fake cpu frequency file for all cpus.
+const char kAllTimeInStateSuffixPath[] = "all_time_in_state";
+
+// The string content of the fake cpu frequency file for cpu0.
+const char kTimeInStateContentCpu0[] =
+    "20000 30000000\n"
+    "60000 90000\n"
+    "100000 50000\n";
+
+// The string content of the fake cpu frequency file for cpu1.
+const char kTimeInStateContentCpu1[] =
+    "20000 31000000\n"
+    "60000 91000\n"
+    "100000 51000\n";
+
+// The string content of the fake cpu frequency file for all cpus.
+const char kAllTimeInStateContent[] =
+    "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";
+
+}  // namespace
+
+class CpuDataCollectorTest : public testing::Test {
+ public:
+  CpuDataCollectorTest()
+      : kExpectedCpuFreqStateNames({"20", "60", "100"}),
+        kExpectedTimeInStateCpu0({TimeDelta::FromMilliseconds(300000000),
+                                  TimeDelta::FromMilliseconds(900000),
+                                  TimeDelta::FromMilliseconds(500000)}),
+        kExpectedTimeInStateCpu1({TimeDelta::FromMilliseconds(310000000),
+                                  TimeDelta::FromMilliseconds(910000),
+                                  TimeDelta::FromMilliseconds(510000)}) {
+    CHECK(temp_dir_.CreateUniqueTempDir());
+
+    time_in_state_path_cpu0_ =
+        temp_dir_.GetPath().AppendASCII(kTimeInStateSuffixPathCpu0);
+    time_in_state_path_cpu1_ =
+        temp_dir_.GetPath().AppendASCII(kTimeInStateSuffixPathCpu1);
+    all_time_in_state_path_ =
+        temp_dir_.GetPath().AppendASCII(kAllTimeInStateSuffixPath);
+
+    CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu0_));
+    CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu1_));
+    CHECK(base::CreateTemporaryFile(&all_time_in_state_path_));
+    CHECK(base::WriteFile(time_in_state_path_cpu0_, kTimeInStateContentCpu0,
+                          static_cast<int>(strlen(kTimeInStateContentCpu0))) !=
+          -1);
+    CHECK(base::WriteFile(time_in_state_path_cpu1_, kTimeInStateContentCpu1,
+                          static_cast<int>(strlen(kTimeInStateContentCpu1))) !=
+          -1);
+    CHECK(base::WriteFile(all_time_in_state_path_, kAllTimeInStateContent,
+                          static_cast<int>(strlen(kAllTimeInStateContent))) !=
+          -1);

[xiyuan, 2017/05/04 20:12:01]

Any reason why not put all of these in an overidden testing::Test::SetUp()? And
CHECK -> ASSERT_TRUE when moving the code.

[weidongg, 2017/05/04 20:32:46]

There are some constants to be initialized in the c'tor, so may be it's better
to put these code here.
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

[xiyuan, 2017/05/04 20:40:02]

The const member are initialized in the member initializer line. Leaving the
code in ctor has no real benefit on that.

The code here is too complicated for a ctor, IMHO. Also you have to use CHECK in
ctor. CHECK should be used with care as it aborts the process and might have
side effects, e.g. generating a core file if we don't config test bots
correctly. I would prefer to move the code into Setup and change CHECK to
ASSERT_TRUE.

[weidongg, 2017/05/04 21:03:45]

Thanks for explanation. Done.

[Daniel Erat, 2017/05/04 21:12:46]

not a huge deal either way, but just to give my justification for using
c'tor/d'tor rather than SetUp/TearDown:

- these commands should never fail. if they do, something is seriously wrong
(e.g. the disk is broken) and other tests are going to fail too.
- it's pointless and confusing to have two places to put init and de-init code.
we often end up with code being split between the c'tor/d'tor and
SetUp/TearDown.
- as mentioned above, the googletest faq says to use c'tor/d'tor unless you have
a good reason not to.
- it's easy to make typos (e.g. "Setup"), although hopefully the compiler
catches this now with 'override'.

so in my opinion, the only reason to use SetUp/TearDown is if you're deriving
from another test class that already uses it and you need to do setup that runs
after the base class.

[xiyuan, 2017/05/04 21:25:35]

My preference is to make ctor as simple as possible to avoid strange and hard to
find bugs such as calling virtual functions from ctor etc. It is not a big deal
for the code here though. That is why it was just a nit.

+  }
+
+ protected:
+  // Expected cpu frequency state names after calling |ReadCpuFreqTimeInState|
+  // or |ReadCpuFreqAllTimeInState|
+  const std::vector<std::string> kExpectedCpuFreqStateNames;
+
+  // Expected time_in_state of sample for cpu0.
+  const std::vector<TimeDelta> kExpectedTimeInStateCpu0;
+
+  // Expected time_in_state of sample for cpu1.
+  const std::vector<TimeDelta> kExpectedTimeInStateCpu1;
+
+  base::ScopedTempDir temp_dir_;
+  base::FilePath time_in_state_path_cpu0_;
+  base::FilePath time_in_state_path_cpu1_;
+  base::FilePath all_time_in_state_path_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CpuDataCollectorTest);
+};
+
+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_, &cpu_freq_state_names, &freq_sample_cpu0);
+  EXPECT_EQ(kExpectedCpuFreqStateNames, cpu_freq_state_names);
+  EXPECT_EQ(kExpectedTimeInStateCpu0, freq_sample_cpu0.time_in_state);
+
+  cpu_freq_state_names.clear();
+  CpuDataCollector::ReadCpuFreqTimeInState(
+      time_in_state_path_cpu1_, &cpu_freq_state_names, &freq_sample_cpu1);
+  EXPECT_EQ(kExpectedCpuFreqStateNames, cpu_freq_state_names);
+  EXPECT_EQ(kExpectedTimeInStateCpu1, 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_, &cpu_freq_state_names, &freq_samples);
+  EXPECT_EQ(kExpectedCpuFreqStateNames, cpu_freq_state_names);
+  EXPECT_EQ(kExpectedTimeInStateCpu0, freq_samples[0].time_in_state);
+  EXPECT_EQ(kExpectedTimeInStateCpu1, freq_samples[1].time_in_state);
+}
+
+}  // namespace chromeos