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

remoting/host/process_stats_sender_unittest.cc

Index: remoting/host/process_stats_sender_unittest.cc
diff --git a/remoting/host/process_stats_sender_unittest.cc b/remoting/host/process_stats_sender_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4e36ee474a163a802aa4ccdf8d2202cfcc449d0c
--- /dev/null
+++ b/remoting/host/process_stats_sender_unittest.cc
@@ -0,0 +1,177 @@
+// 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>

[joedow, 2017/04/24 17:16:09]

nit: #include <cstdint> instead of old C style header.

[Hzj_jie, 2017/04/25 00:17:54]

It looks like stdint.h is preferred in Chromium: it defines types in global
namespace instead of std.
https://cs.chromium.org/search/?q=include.*cstdint+package:%5Echromium$&type=cs
returns 80 results.
https://cs.chromium.org/search/?q=include.*stdint%5C.h+package:%5Echromium$&t...
returns 12600 results.
I do see some files using std::uint32_t, but it's really not popular.
https://cs.chromium.org/search/?q=std::uint32_t+package:%5Echromium$&type=cs
returns 18 results.

[joedow, 2017/04/25 15:57:28]

It is one of the newly allowed C++11 features
(https://chromium-cpp.appspot.com/) so I'm not surprised that older code doesn't
use it.

I'd prefer C++ headers over old c-style, also I can't think of a downside of
using the new header either.

[Hzj_jie, 2017/04/25 22:19:19]

I believe it's an allowed feature, but a standard implementation of cstdint
equals to
namespace std {
#include <stdint.h>
}  // namespace std
http://en.cppreference.com/w/cpp/header/cstdint
v.s.
http://en.cppreference.com/w/c/types/integer
So it means we need to change all int32_t to std::int32_t in this file. I
believe some files mistakenly use cstdint with ::int32_t, but it's not standard.

+
+#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/proto/process_stats.pb.h"
+#include "remoting/protocol/process_stats_stub.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+class MockProcessStatsStub : public protocol::ProcessStatsStub {

[joedow, 2017/04/24 17:16:09]

Rename MockProcessStatsStub to FakeProcessStatsStub (or similar) as it isn't a
mock.

[Hzj_jie, 2017/04/25 00:17:54]

Oh, you have made two similar comments in my two changes. Would you mind to give
me more hints why fake is preferred here than mock? (My pool English skill tells
me both of these words have the meaning of "not real".)

[joedow, 2017/04/25 15:57:28]

They have different meanings when it comes to testing (for instance
https://www.martinfowler.com/articles/mocksArentStubs.html).  

The gist is that a mock object (in chromium these derive from GMock classes)
must be set up with call expectations and possibly with default return values.

A Fake object has a simplified impl of the real object.

A stub is like a mock but doesn't care about which methods were called.

This class feels like a Fake since it receives the usages but does nothing with
them except provide them for testing.

[Hzj_jie, 2017/04/25 22:19:19]

Thank you for the explanation, very clear.

+ public:
+  MockProcessStatsStub() = default;
+  ~MockProcessStatsStub() override = default;
+
+  void OnProcessStats(
+      const protocol::AggregatedProcessResourceUsage& usage) override {
+    received_.push_back(usage);
+  }
+
+  const std::vector<protocol::AggregatedProcessResourceUsage>& received()
+      const {
+    return received_;
+  }
+
+  void Clear() { received_.clear(); }
+
+ private:
+  std::vector<protocol::AggregatedProcessResourceUsage> received_;
+};
+
+class MockProcessStatsAgent : public ProcessStatsAgent {

[joedow, 2017/04/24 17:16:09]

Rename to FakeProcessStatsAgent or TestProcessStatsAgent

[Hzj_jie, 2017/04/25 00:17:54]

Done.

+ public:
+  MockProcessStatsAgent() = default;
+  ~MockProcessStatsAgent() override = default;
+
+  protocol::ProcessResourceUsage GetResourceUsage() override {
+    protocol::ProcessResourceUsage usage;
+    usage.set_process_name("MockProcessStatsAgent");
+    usage.set_processor_usage(index_);
+    usage.set_working_set_size(index_);
+    usage.set_pagefile_size(index_);
+    index_++;
+    return usage;
+  }
+
+  // Asserts the input |usage| contains data generated by the |index|-th (starts
+  // from 0) GetResourceUsage().

[joedow, 2017/04/24 17:16:09]

I'm a little confused by this term '|index|-th', maybe clean the comment up to
something like "Checks the expected usage based on index"?

[Hzj_jie, 2017/04/25 00:17:54]

Done.

+  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);
+  }
+
+  size_t issued_times() const {
+    return index_;
+  }
+
+ private:
+  size_t index_ = 0;
+};
+
+}  // namespace
+
+TEST(ProcessStatsSenderTest, ReportUsage) {
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  MockProcessStatsStub stub;
+  std::unique_ptr<ProcessStatsSender> stats;
+  // Owned by |stats|.
+  MockProcessStatsAgent* agent = new MockProcessStatsAgent();
+  message_loop.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          [](std::unique_ptr<ProcessStatsSender>* stats,
+             MockProcessStatsStub* stub,
+             MockProcessStatsAgent* 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)));
+  message_loop.task_runner()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind([](std::unique_ptr<ProcessStatsSender>* stats,
+                    const MockProcessStatsStub& stub,
+                    const MockProcessStatsAgent& agent) -> void {
+                   ASSERT_EQ(stub.received().size(), agent.issued_times());
+                   stats->reset();
+                 },
+                 base::Unretained(&stats),
+                 base::ConstRef(stub),
+                 base::ConstRef(*agent)),
+      base::TimeDelta::FromMilliseconds(100));

[joedow, 2017/04/24 17:16:09]

You should avoid relying on intervals (i.e. 100ms) in unit tests.  Your tests
will run on a variety of machines and these can cause inadvertent race
conditions.

[Hzj_jie, 2017/04/25 00:17:54]

It's a little bit hard to do so. Indeed this test does not rely on this time
interval, but only depends on a sequential task runner, and the first PostTask
won't spend 100 ms.
Here the trouble is we need to test that the "new ProcessStatsSender" starts the
sending process. But the only place we can get the count is in the callback of
the first PostDelayedTask call. assert_eq(stub.received().size(),
stats->time_calls()) does not really help: the sending process may not start at
all and causes all following assertions to be ignored.

Do you have a better solution?

[joedow, 2017/04/25 15:57:28]

It might spend 100ms if it was running on a machine with heavy CPU usage and one
of the thread/memory sanitizers running.

We can talk offline if you'd like, but it is important to make sure these tests
are solid as you don't want to have to deal with flakiness on the TSAN/ASAN
bots.

[Hzj_jie, 2017/04/25 22:19:19]

I agree. A simple choice is to remove the ASSERT_GT in line 119. But it means
part of the logic has not been tested.
We may talk offline.

+  message_loop.task_runner()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(),
+      base::TimeDelta::FromMilliseconds(101));
+  run_loop.Run();
+
+  ASSERT_GT(stub.received().size(), 0U);

[joedow, 2017/04/24 17:16:09]

Can you assert a specific value (rather than a generic greater than check)?  I
think it would be better to make this test deterministic rather than rely on a
timeout (100ms).

[Hzj_jie, 2017/04/25 00:17:54]

See my previous comment.

+  for (size_t i = 0; i < stub.received().size(); i++) {
+    MockProcessStatsAgent::AssertExpected(stub.received()[i], i);
+  }
+}
+
+TEST(ProcessStatsSenderTest, MergeUsage) {
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  MockProcessStatsStub stub;
+  std::unique_ptr<ProcessStatsSender> stats;
+  // Owned by |stats|.
+  MockProcessStatsAgent* agent1 = new MockProcessStatsAgent();
+  MockProcessStatsAgent* agent2 = new MockProcessStatsAgent();
+  message_loop.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          [](std::unique_ptr<ProcessStatsSender>* stats,
+             MockProcessStatsStub* stub,
+             MockProcessStatsAgent* agent1,
+             MockProcessStatsAgent* 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)));
+  message_loop.task_runner()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind([](std::unique_ptr<ProcessStatsSender>* stats,
+                    const MockProcessStatsStub& stub,
+                    const MockProcessStatsAgent& agent1,
+                    const MockProcessStatsAgent& agent2) -> void {
+                   ASSERT_EQ(stub.received().size(), agent1.issued_times());
+                   ASSERT_EQ(stub.received().size(), agent2.issued_times());
+                   stats->reset();
+                 },
+                 base::Unretained(&stats),
+                 base::ConstRef(stub),
+                 base::ConstRef(*agent1),
+                 base::ConstRef(*agent2)),
+      base::TimeDelta::FromMilliseconds(100));
+  message_loop.task_runner()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(),
+      base::TimeDelta::FromMilliseconds(101));
+  run_loop.Run();
+
+  ASSERT_GT(stub.received().size(), 0U);
+  for (size_t i = 0; i < stub.received().size(); i++) {
+    MockProcessStatsAgent::AssertExpected(stub.received()[i], i * 2);

[joedow, 2017/04/24 17:16:09]

IIUC you are verifying the aggregate, but can you also verify the individual
payloads?

[Hzj_jie, 2017/04/25 00:17:54]

I think it's covered by ReportUsage test already. I am happy to add it.

[joedow, 2017/04/25 15:57:28]

Thanks for adding it.  It isn't necessarily covered by the previous test as you
only had one usage agent before, not two.

[Hzj_jie, 2017/04/25 22:19:19]

Oh, reasonable.

+  }
+}
+
+}  // namespace remoting