metrics: Add dbus interface for GetRandomPerfOutput

chrome/browser/metrics/perf_provider_chromeos_unittest.cc

+// Copyright 2015 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 "chrome/browser/metrics/perf_provider_chromeos.h"
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/login/login_state.h"
+#include "components/metrics/proto/sampled_profile.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace metrics {
+
+namespace {
+
+// Returns an example PerfDataProto. The contents don't have to make sense. They
+// just need to constitute a semantically valid protobuf.
+// |result| is an output parameter that will contain the created protobuf.
+// |result_raw| is |result| in serialized format.
+void GetExamplePerfDataProto(PerfDataProto* result,
+                             std::vector<uint8_t>* result_raw) {
+  PerfDataProto& proto = *result;

[Ilya Sherman, 2015/07/01 03:09:03]

Again, why don't you use the arrow operator?  It can sometimes be clearer to use
a reference than a longer name, but in this case, there's really no increased
clarity to using the reference.

[Simon Que, 2015/07/01 22:14:11]

Done.

+  proto.set_timestamp_sec(1435604013);  // Time since epoch in seconds.
+
+  PerfDataProto_PerfFileAttr& file_attr = *proto.add_file_attrs();

[Ilya Sherman, 2015/07/01 03:09:02]

I'd use a pointer here, too.  In general, bare references are less commonly used
than pointers.  Mostly, they're used where STL functions return bare references,
e.g. std::vector::operator[].

[Simon Que, 2015/07/01 22:14:11]

Done.

+  file_attr.add_ids(61);
+  file_attr.add_ids(62);
+  file_attr.add_ids(63);
+
+  PerfDataProto_PerfEventAttr& attr = *file_attr.mutable_attr();
+  attr.set_type(1);
+  attr.set_size(2);
+  attr.set_config(3);
+  attr.set_sample_period(4);
+  attr.set_sample_freq(5);
+
+  PerfDataProto_PerfEventStats& stats = *proto.mutable_stats();
+  stats.set_num_events_read(100);
+  stats.set_num_sample_events(200);
+  stats.set_num_mmap_events(300);
+  stats.set_num_fork_events(400);
+  stats.set_num_exit_events(500);
+
+  std::string result_string;
+  ASSERT_TRUE(proto.SerializeToString(&result_string));
+  *result_raw =
+      std::vector<uint8_t>(result_string.begin(), result_string.end());
+}
+
+// Returns an example PerfStatProto. The contents don't have to make sense. They
+// just need to constitute a semantically valid protobuf.
+// |result| is an output parameter that will contain the created protobuf.
+// |result_raw| is |result| in serialized format.
+void GetExamplePerfStatProto(PerfStatProto* result,
+                             std::vector<uint8_t>* result_raw) {
+  PerfStatProto& proto = *result;
+  proto.set_command_line(
+      "perf stat -a -e cycles -e instructions -e branches -- sleep 2");
+
+  PerfStatProto_PerfStatLine& line1 = *proto.add_line();
+  line1.set_time_ms(1000);
+  line1.set_count(2000);
+  line1.set_event("cycles");
+
+  PerfStatProto_PerfStatLine& line2 = *proto.add_line();
+  line2.set_time_ms(2000);
+  line2.set_count(5678);
+  line2.set_event("instructions");
+
+  PerfStatProto_PerfStatLine& line3 = *proto.add_line();
+  line3.set_time_ms(3000);
+  line3.set_count(9999);
+  line3.set_event("branches");
+
+  std::string result_string;
+  ASSERT_TRUE(proto.SerializeToString(&result_string));
+  *result_raw =
+      std::vector<uint8_t>(result_string.begin(), result_string.end());

[Ilya Sherman, 2015/07/01 03:09:03]

Why not either (a) return a string, or (b) call SerializeMessageToVector, which
you've defined below?

[Simon Que, 2015/07/01 22:14:11]

Done.

+}
+
+std::vector<uint8_t> SerializeMessageToVector(
+    const google::protobuf::MessageLite& message) {
+  std::vector<uint8_t> result(message.ByteSize());
+  message.SerializeToArray(result.data(), result.size());
+  return result;
+}
+
+}  // namespace
+
+using IncognitoObserverInterface = PerfProvider::IncognitoObserverInterface;
+
+class PerfProviderTest : public testing::Test {
+ public:
+  // Allows testing of PerfProvider behavior when an incognito window is opened.
+  class MockIncognitoObserver : public IncognitoObserverInterface {

[Ilya Sherman, 2015/07/01 03:09:03]

nit: Please name this "TestIncognitoObserver" or "FakeIncognitoObserver" --
there aren't any mocked methods here, in the gMock sense, which is what
"MockFoo" classes usually refer to.

[Simon Que, 2015/07/01 21:08:03]

Done.

+   public:
+    MockIncognitoObserver() : incognito_launched_(false) {}
+
+    ~MockIncognitoObserver() {}
+
+    bool incognito_launched() const override {
+      return incognito_launched_;
+    }
+    void set_incognito_launched(bool value) {
+      incognito_launched_ = value;
+    }
+
+   private:
+    bool incognito_launched_;
+  };

[Ilya Sherman, 2015/07/01 03:09:02]

nit: Please move this class to the anonymous namespace, rather than defining it
as an inner class.

[Simon Que, 2015/07/01 21:08:02]

Done.

+
+  PerfProviderTest() {
+    GetExamplePerfDataProto(&perf_data_proto_, &perf_data_raw_);
+    GetExamplePerfStatProto(&perf_stat_proto_, &perf_stat_raw_);
+  }
+
+  void SetUp() override {
+    // PerfProvider requires chromeos::LoginState and
+    // chromeos::DBusThreadManagerto be initialized.
+    chromeos::LoginState::Initialize();
+    chromeos::DBusThreadManager::Initialize();
+
+    perf_provider_.reset(new PerfProvider);
+    EXPECT_TRUE(cached_perf_data().empty());

[Ilya Sherman, 2015/07/01 03:09:03]

Rather than running this in the setup for each test case, please create a
separate (tiny) test case that tests just this one thing.

[Simon Que, 2015/07/01 22:14:11]

Done.

+
+    EXPECT_FALSE(incognito_observer_.incognito_launched());\

[Ilya Sherman, 2015/07/01 03:09:03]

It's a little weird to be setting expectations like this in the setup code,
since it's not really expectations about the class under test.

[Ilya Sherman, 2015/07/01 03:09:03]

nit: Please remove the trailing "\"

[Simon Que, 2015/07/01 22:14:11]

Done.

[Simon Que, 2015/07/01 22:14:11]

Done.

+
+    sampled_profile_.reset(new SampledProfile);
+    sampled_profile_->set_trigger_event(SampledProfile::PERIODIC_COLLECTION);

[Ilya Sherman, 2015/07/01 03:09:02]

Could all of this be done in the constructor, and all of TearDown() be done in
the destructor?

+  }
+
+  void TearDown() override {
+    perf_provider_.reset(NULL);

[Ilya Sherman, 2015/07/01 03:09:02]

nit: Just reset() is fine; but FYI, "nullptr" is preferred to "NULL", as it is
more strongly typed.

[Simon Que, 2015/07/01 22:14:11]

Done.

+    chromeos::DBusThreadManager::Shutdown();
+    chromeos::LoginState::Shutdown();
+
+    incognito_observer_.set_incognito_launched(false);

[Ilya Sherman, 2015/07/01 03:09:03]

No need to do this, as the observer will be recreated for each test.

[Simon Que, 2015/07/01 22:14:11]

Done.

+  }
+
+ protected:
+  // Accessor for derived classes to access |PerfProvider::cached_perf_data_|.
+  const std::vector<SampledProfile>& cached_perf_data() const {
+    return perf_provider_->cached_perf_data_;
+  }

[Ilya Sherman, 2015/07/01 03:09:02]

nit: Tests should just access this directly.  If the issue is that the cached
perf data is private, think about whether it's really appropriate for tests to
be accessing it in the first place.  Most likely not -- it's better to test the
public API, i.e. GetSampledProfiles().

[Simon Que, 2015/07/01 22:14:11]

Done.

+
+  // Accessor for derived classes to call PerfProvider::ParseOutputProtoIfValid.
+  void ParseOutputProtoIfValid(
+      const MockIncognitoObserver& incognito_observer,
+      const SampledProfile& sampled_profile,

[Ilya Sherman, 2015/07/01 03:09:03]

Why not just pass scoped_ptr's to this function directly?

[Simon Que, 2015/07/01 22:14:11]

Done.

+      int result,
+      const std::vector<uint8>& perf_data,
+      const std::vector<uint8>& perf_stat) {
+    // Create copies of |incognito_observer| and |sampled_profile| to pass to
+    // PerfProvider::ParseOutputProtoIfValid.
+    MockIncognitoObserver* incognito_observer_copy = new MockIncognitoObserver;

[Ilya Sherman, 2015/07/01 03:09:03]

nit: Please declare this as a scoped ptr.  Also, can you not combine the
construction and the assignment, i.e. by calling the copy constructor here?

[Simon Que, 2015/07/01 22:14:11]

Done.

+    SampledProfile* sampled_profile_copy = new SampledProfile;

[Ilya Sherman, 2015/07/01 03:09:02]

Ditto.

[Simon Que, 2015/07/01 22:14:11]

Done

+
+    *incognito_observer_copy = incognito_observer;
+    // The contents of the protobuf cannot be copied using a simple assignment.
+    EXPECT_TRUE(sampled_profile_copy->ParseFromString(
+        sampled_profile.SerializeAsString()));
+
+    perf_provider_->PerfProvider::ParseOutputProtoIfValid(
+        scoped_ptr<IncognitoObserverInterface>(incognito_observer_copy).Pass(),
+        scoped_ptr<SampledProfile>(sampled_profile_copy).Pass(),
+        result,
+        perf_data,
+        perf_stat);
+  }
+
+  scoped_ptr<PerfProvider> perf_provider_;
+
+  // For simulating whether an incognito window is open.
+  MockIncognitoObserver incognito_observer_;
+
+  // The default template for the SampledProfile that will contain the incoming
+  // perf protobuf. Can be modified between calls to ParseOutputProtoIfValid().
+  // SetUp() resets it to the default value.

[Ilya Sherman, 2015/07/01 03:09:03]

The constructor runs precisely as often as SetUp(), so there's no sense in which
SetUp() resets this value.

[Simon Que, 2015/07/01 22:14:11]

Acknowledged.

+  scoped_ptr<SampledProfile> sampled_profile_;

[Ilya Sherman, 2015/07/01 03:09:02]

IMO it would be clearer not to define this as part of the fixture class at all,
and instead to simply have each test create the SampledProfile that it needs. 
You could have a helper function to create the default SampledProfile if you
really want, but it doesn't look like that would save much code, since the
default SampledProfile seems to only have a single field set.

[Simon Que, 2015/07/01 22:14:11]

Done.

+
+  // These store example perf data/stat protobufs for testing.
+  PerfDataProto perf_data_proto_;
+  PerfStatProto perf_stat_proto_;
+  // These are the equivalents of the above protobufs as serialized raw data.
+  std::vector<uint8_t> perf_data_raw_;
+  std::vector<uint8_t> perf_stat_raw_;

[Ilya Sherman, 2015/07/01 03:09:03]

Why do you need both variants here?

[Simon Que, 2015/07/01 22:14:11]

It's used by BothPerfDataProtoAndPerfStatProto().

+
+  DISALLOW_COPY_AND_ASSIGN(PerfProviderTest);
+};
+
+TEST_F(PerfProviderTest, PerfDataProtoOnly) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,    // Zero means successful.

[Ilya Sherman, 2015/07/01 03:09:03]

nit: Perhaps define constants like "const int kSuccesss = 0" at the top of the
file.

[Simon Que, 2015/07/01 22:14:11]

Done.

+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  ASSERT_EQ(1U, cached_perf_data().size());
+
+  const SampledProfile& profile = cached_perf_data()[0];
+  EXPECT_EQ(SampledProfile::PERIODIC_COLLECTION, profile.trigger_event());
+  EXPECT_GT(profile.ms_after_login(), 0);
+
+  ASSERT_TRUE(profile.has_perf_data());
+  EXPECT_FALSE(profile.has_perf_stat());
+  EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile.perf_data()));
+}
+
+TEST_F(PerfProviderTest, PerfStatProtoOnly) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          std::vector<uint8_t>(),
+                          perf_stat_raw_);
+  ASSERT_EQ(1U, cached_perf_data().size());
+
+  const SampledProfile& profile = cached_perf_data()[0];
+  EXPECT_EQ(SampledProfile::PERIODIC_COLLECTION, profile.trigger_event());
+  EXPECT_GT(profile.ms_after_login(), 0);
+
+  EXPECT_FALSE(profile.has_perf_data());
+  ASSERT_TRUE(profile.has_perf_stat());
+  EXPECT_EQ(perf_stat_raw_, SerializeMessageToVector(profile.perf_stat()));
+}
+
+TEST_F(PerfProviderTest, BothPerfDataProtoAndPerfStatProto) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          perf_stat_raw_);
+  ASSERT_TRUE(cached_perf_data().empty());
+}
+
+TEST_F(PerfProviderTest, InvalidPerfOutputResult) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          1,    // Nonzero means unsuccessful.
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+
+  // Should not have been stored.
+  EXPECT_TRUE(cached_perf_data().empty());
+}
+
+// Change |sampled_profile_| between calls to ParseOutputProtoIfValid().
+TEST_F(PerfProviderTest, MultipleCalls) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  EXPECT_EQ(1U, cached_perf_data().size());
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::RESTORE_SESSION);
+  sampled_profile_->set_ms_after_login(23456);
+  sampled_profile_->set_ms_after_restore(3000);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          std::vector<uint8_t>(),
+                          perf_stat_raw_);
+  EXPECT_EQ(2U, cached_perf_data().size());
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::RESUME_FROM_SUSPEND);
+  sampled_profile_->set_ms_after_login(34567);
+  sampled_profile_->set_suspend_duration_ms(60000);
+  sampled_profile_->set_ms_after_resume(1500);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  EXPECT_EQ(3U, cached_perf_data().size());
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::PERIODIC_COLLECTION);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          std::vector<uint8_t>(),
+                          perf_stat_raw_);
+  ASSERT_EQ(4U, cached_perf_data().size());
+
+  const SampledProfile& profile1 = cached_perf_data()[0];
+  EXPECT_EQ(SampledProfile::PERIODIC_COLLECTION, profile1.trigger_event());
+  EXPECT_GT(profile1.ms_after_login(), 0);
+  ASSERT_TRUE(profile1.has_perf_data());
+  EXPECT_FALSE(profile1.has_perf_stat());
+  EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile1.perf_data()));
+
+  const SampledProfile& profile2 = cached_perf_data()[1];
+  EXPECT_EQ(SampledProfile::RESTORE_SESSION, profile2.trigger_event());
+  EXPECT_GT(profile2.ms_after_login(), 0);
+  EXPECT_EQ(3000, profile2.ms_after_restore());
+  EXPECT_FALSE(profile2.has_perf_data());
+  ASSERT_TRUE(profile2.has_perf_stat());
+  EXPECT_EQ(perf_stat_raw_, SerializeMessageToVector(profile2.perf_stat()));
+
+  const SampledProfile& profile3 = cached_perf_data()[2];
+  EXPECT_EQ(SampledProfile::RESUME_FROM_SUSPEND, profile3.trigger_event());
+  EXPECT_GT(profile3.ms_after_login(), 0);
+  EXPECT_EQ(60000, profile3.suspend_duration_ms());
+  EXPECT_EQ(1500, profile3.ms_after_resume());
+  ASSERT_TRUE(profile3.has_perf_data());
+  EXPECT_FALSE(profile3.has_perf_stat());
+  EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile3.perf_data()));
+
+  const SampledProfile& profile4 = cached_perf_data()[3];
+  EXPECT_EQ(SampledProfile::PERIODIC_COLLECTION, profile4.trigger_event());
+  EXPECT_GT(profile4.ms_after_login(), 0);
+  EXPECT_FALSE(profile4.has_perf_data());
+  ASSERT_TRUE(profile4.has_perf_stat());
+  EXPECT_EQ(perf_stat_raw_, SerializeMessageToVector(profile4.perf_stat()));
+}
+
+// Simulate opening and closing of incognito window in between calls to
+// ParseOutputProtoIfValid().
+TEST_F(PerfProviderTest, IncognitoWindowOpened) {
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  EXPECT_EQ(1U, cached_perf_data().size());
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::RESTORE_SESSION);
+  sampled_profile_->set_ms_after_login(23456);
+  sampled_profile_->set_ms_after_restore(3000);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          std::vector<uint8_t>(),
+                          perf_stat_raw_);
+  EXPECT_EQ(2U, cached_perf_data().size());
+
+  // An incognito window opens.
+  incognito_observer_.set_incognito_launched(true);
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::RESUME_FROM_SUSPEND);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  EXPECT_EQ(2U, cached_perf_data().size());
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::PERIODIC_COLLECTION);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          std::vector<uint8_t>(),
+                          perf_stat_raw_);
+  ASSERT_EQ(2U, cached_perf_data().size());
+
+  // Incognito window closes, should be good to go again.
+  incognito_observer_.set_incognito_launched(false);
+
+  sampled_profile_.reset(new SampledProfile);
+  sampled_profile_->set_trigger_event(SampledProfile::RESUME_FROM_SUSPEND);
+  sampled_profile_->set_ms_after_login(34567);
+  sampled_profile_->set_suspend_duration_ms(60000);
+  sampled_profile_->set_ms_after_resume(1500);
+  ParseOutputProtoIfValid(incognito_observer_,
+                          *sampled_profile_,
+                          0,
+                          perf_data_raw_,
+                          std::vector<uint8_t>());
+  EXPECT_EQ(3U, cached_perf_data().size());
+
+  const SampledProfile& profile1 = cached_perf_data()[0];
+  EXPECT_EQ(SampledProfile::PERIODIC_COLLECTION, profile1.trigger_event());
+  EXPECT_GT(profile1.ms_after_login(), 0);
+  ASSERT_TRUE(profile1.has_perf_data());
+  EXPECT_FALSE(profile1.has_perf_stat());
+  EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile1.perf_data()));
+
+  const SampledProfile& profile2 = cached_perf_data()[1];
+  EXPECT_EQ(SampledProfile::RESTORE_SESSION, profile2.trigger_event());
+  EXPECT_GT(profile2.ms_after_login(), 0);
+  EXPECT_EQ(3000, profile2.ms_after_restore());
+  EXPECT_FALSE(profile2.has_perf_data());
+  ASSERT_TRUE(profile2.has_perf_stat());
+  EXPECT_EQ(perf_stat_raw_, SerializeMessageToVector(profile2.perf_stat()));
+
+  const SampledProfile& profile3 = cached_perf_data()[2];
+  EXPECT_EQ(SampledProfile::RESUME_FROM_SUSPEND, profile3.trigger_event());
+  EXPECT_GT(profile3.ms_after_login(), 0);
+  EXPECT_EQ(60000, profile3.suspend_duration_ms());
+  EXPECT_EQ(1500, profile3.ms_after_resume());
+  ASSERT_TRUE(profile3.has_perf_data());
+  EXPECT_FALSE(profile3.has_perf_stat());
+  EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile3.perf_data()));
+}
+
+}  // namespace metrics