[Chromoting] Retrieve process resource usage (ProcessStats) and its tests

remoting/host/process_stats_sender_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 "remoting/host/process_stats_sender.h"
+
+#include <stdint.h>
+
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "remoting/host/process_stats_agent.h"
+#include "remoting/proto/process_stats.pb.h"
+#include "remoting/protocol/process_stats_stub.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+class FakeProcessStatsStub : public protocol::ProcessStatsStub {
+ public:
+  FakeProcessStatsStub() = default;
+  ~FakeProcessStatsStub() override = default;
+
+  void OnProcessStats(
+      const protocol::AggregatedProcessResourceUsage& usage) override {
+    received_.push_back(usage);
+    if (received_.size() >= static_cast<size_t>(expected_usage_received_) &&

[joedow, 2017/04/26 16:44:06]

Would this ever be a problem if you expected 2 usage structs but instead of one
call with 2 you received 2 calls with 1 struct each?  You may not but if that is
the case then you would want to use a current_usage_count_ to compare against.

[Hzj_jie, 2017/04/26 20:47:19]

No, I do not think it's possible, since we push_back one usage per
OnProcessStats() call. I would change this line into DCHECK_LE(received_.size(),
static_cast<size_t>(expected_usage_received_)).
And also, I believe checking whether quit_closure_.is_null() is not necessary:
tests should always call SetQuitWhen(). Instead of testing the behavior of the
ProcessStatsSender, these ifs are checking the behavior of the
FakeProcessStatsStub itself.

+        !quit_closure_.is_null()) {
+      quit_closure_.Run();

[joedow, 2017/04/26 16:44:06]

Do you want to immediately run this closure?  You might be safer posting it to
the task_runner (that way the method that calls this function can return and
clean up before the test runs more code).

[Hzj_jie, 2017/04/26 20:47:19]

I think running it immediately should be a better choice, otherwise we may
encounter some uncertainties whether the quit_closure_ or the next
ProcessStatsSender::Report would be run first.

[joedow, 2017/04/26 23:48:01]

This callback is only used for testing so I don't see a benefit for calling it
directly.  The common pattern is to post the resume closure so the previous code
can finish.

Not doing so can lead (though not in this specific CL) to your test instance
being torn down while it is running (if an ASSERT FAILS) for instance.

[Hzj_jie, 2017/04/27 02:21:33]

Sorry, I have not followed. IMO, if the quit_closure_ is posted to the task
runner but after the next Report() function, i.e. this line uses over 1
millisecond, we will have 11 samples instead of 10. RepeatedTimer schedules next
task before running current one.
(https://cs.chromium.org/chromium/src/base/timer/timer.cc?rcl=65f71c8b5c87a14a...)

+    }
+  }
+
+  const std::vector<protocol::AggregatedProcessResourceUsage>& received()
+      const {
+    return received_;
+  }
+
+  void SetQuitWhen(int expected_usage_received, base::Closure quit_closure) {

[joedow, 2017/04/26 16:44:06]

The names here are a little confusing.  Instead of having this method, I think
it would be better to have individual setters for |quit_closure_| and
|expected_usage_received_|.  If you default |expected_usage_received_| to 1,
then you'd only need to set that value in tests where you have multiple agents
sending usage data.

[Hzj_jie, 2017/04/26 20:47:20]

Done.
But no matter how many agents, the stub receives consistent count of
AggregatedProcessResourceUsage.

+    expected_usage_received_ = expected_usage_received;

[joedow, 2017/04/26 16:44:06]

nit: rename expected_usage_received_ to expected_usage_count_ ?

[Hzj_jie, 2017/04/26 20:47:19]

Done.

+    quit_closure_ = quit_closure;
+  }
+
+ private:
+  std::vector<protocol::AggregatedProcessResourceUsage> received_;
+  int expected_usage_received_ = 0;

[joedow, 2017/04/26 16:44:06]

If this member were a size_t, could you remove the cast above?

[Hzj_jie, 2017/04/26 20:47:20]

I believe the code style discourages the usage of size_t (unsigned). But
unfortunately, std::vector::size() returns size_t.

[joedow, 2017/04/26 23:48:01]

Coding style discourages it if your intention is to show that a value should
never be negative.  It is fine if you need to interop with methods that return
it.  I would switch it an remove the casts above.

[Hzj_jie, 2017/04/27 02:21:33]

Done.

+  base::Closure quit_closure_;
+};
+
+class FakeProcessStatsAgent : public ProcessStatsAgent {
+ public:
+  FakeProcessStatsAgent() = default;
+  ~FakeProcessStatsAgent() override = default;
+
+  protocol::ProcessResourceUsage GetResourceUsage() override {
+    protocol::ProcessResourceUsage usage;
+    usage.set_process_name("FakeProcessStatsAgent");
+    usage.set_processor_usage(index_);
+    usage.set_working_set_size(index_);
+    usage.set_pagefile_size(index_);
+    index_++;
+    return usage;
+  }
+
+  // Checks the expected usage based on index.
+  static void AssertExpected(
+      const protocol::AggregatedProcessResourceUsage& usage,
+      size_t index) {
+    ASSERT_EQ(usage.processor_usage(), index);
+    ASSERT_EQ(usage.working_set_size(), index);
+    ASSERT_EQ(usage.pagefile_size(), index);
+  }
+
+  static void AssertExpected(
+      const protocol::ProcessResourceUsage& usage,
+      size_t index) {
+    ASSERT_EQ(usage.processor_usage(), index);
+    ASSERT_EQ(usage.working_set_size(), index);
+    ASSERT_EQ(usage.pagefile_size(), index);
+  }
+
+  size_t issued_times() const {
+    return index_;
+  }
+
+ private:
+  size_t index_ = 0;
+};
+
+}  // namespace
+
+TEST(ProcessStatsSenderTest, ReportUsage) {
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  FakeProcessStatsStub stub;
+  std::unique_ptr<ProcessStatsSender> stats;
+  // Owned by |stats|.
+  FakeProcessStatsAgent* agent = new FakeProcessStatsAgent();
+
+  stub.SetQuitWhen(10,
+      base::Bind([](std::unique_ptr<ProcessStatsSender>* stats,
+                    const FakeProcessStatsStub& stub,
+                    const FakeProcessStatsAgent& agent,
+                    base::RunLoop* run_loop) -> void {
+                   ASSERT_EQ(stub.received().size(), agent.issued_times());
+                   stats->reset();
+                   run_loop->Quit();
+                 },
+                 base::Unretained(&stats),
+                 base::ConstRef(stub),
+                 base::ConstRef(*agent),
+                 base::Unretained(&run_loop)));
+  message_loop.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          [](std::unique_ptr<ProcessStatsSender>* stats,
+             FakeProcessStatsStub* stub,
+             FakeProcessStatsAgent* agent) -> void {
+            stats->reset(new ProcessStatsSender(
+                stub, base::TimeDelta::FromMilliseconds(1)));
+            (*stats)->AddProcessStatsAgent(
+                std::unique_ptr<ProcessStatsAgent>(agent));
+          },
+          base::Unretained(&stats),
+          base::Unretained(&stub),
+          base::Unretained(agent)));
+  run_loop.Run();
+
+  ASSERT_GT(stub.received().size(), 0U);

[joedow, 2017/04/26 16:44:06]

You can use a specific comparison now (since you aren't relying on a timeout),
can you update these checks?

[Hzj_jie, 2017/04/26 20:47:19]

Sorry, I simply forgot to update it.

+  for (size_t i = 0; i < stub.received().size(); i++) {
+    FakeProcessStatsAgent::AssertExpected(stub.received()[i], i);
+  }
+}
+
+TEST(ProcessStatsSenderTest, MergeUsage) {
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  FakeProcessStatsStub stub;
+  std::unique_ptr<ProcessStatsSender> stats;
+  // Owned by |stats|.
+  FakeProcessStatsAgent* agent1 = new FakeProcessStatsAgent();
+  FakeProcessStatsAgent* agent2 = new FakeProcessStatsAgent();
+
+  stub.SetQuitWhen(10,
+      base::Bind([](std::unique_ptr<ProcessStatsSender>* stats,
+                    const FakeProcessStatsStub& stub,
+                    const FakeProcessStatsAgent& agent1,
+                    const FakeProcessStatsAgent& agent2,
+                    base::RunLoop* run_loop) -> void {
+                   ASSERT_EQ(stub.received().size(), agent1.issued_times());
+                   ASSERT_EQ(stub.received().size(), agent2.issued_times());
+                   stats->reset();
+                   run_loop->Quit();
+                 },
+                 base::Unretained(&stats),
+                 base::ConstRef(stub),
+                 base::ConstRef(*agent1),
+                 base::ConstRef(*agent2),
+                 base::Unretained(&run_loop)));
+  message_loop.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          [](std::unique_ptr<ProcessStatsSender>* stats,
+             FakeProcessStatsStub* stub,
+             FakeProcessStatsAgent* agent1,
+             FakeProcessStatsAgent* agent2) -> void {
+            stats->reset(new ProcessStatsSender(
+                stub, base::TimeDelta::FromMilliseconds(1)));
+            (*stats)->AddProcessStatsAgent(
+                std::unique_ptr<ProcessStatsAgent>(agent1));
+            (*stats)->AddProcessStatsAgent(
+                std::unique_ptr<ProcessStatsAgent>(agent2));
+          },
+          base::Unretained(&stats),
+          base::Unretained(&stub),
+          base::Unretained(agent1),
+          base::Unretained(agent2)));
+  run_loop.Run();
+
+  ASSERT_GT(stub.received().size(), 0U);
+  for (size_t i = 0; i < stub.received().size(); i++) {
+    FakeProcessStatsAgent::AssertExpected(stub.received()[i], i * 2);
+    for (int j = 0; j < stub.received()[i].usages_size(); j++) {
+      FakeProcessStatsAgent::AssertExpected(
+          stub.received()[i].usages().Get(j), i);
+    }
+  }
+}
+
+}  // namespace remoting