memory-infra: Add unittest for MemoryDumpManager summary calculations

base/trace_event/memory_dump_manager_unittest.cc

Index: base/trace_event/memory_dump_manager_unittest.cc
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 732b5c8086652d9d7be8724e60db8c546e96c16b..ad62e58d6c4ba8165d56825532f0f210334a63c1 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -118,7 +118,6 @@ void PostTaskAndWait(const tracked_objects::Location& from_here,
 
 // Adapts a ProcessMemoryDumpCallback into a GlobalMemoryDumpCallback by
 // trimming off the result argument and calling the global callback.
-// TODO (fmeawad): we should keep the results for verification, but currently
 // all results are empty.

[Primiano Tucci (use gerrit), 2017/04/28 15:04:33]

are you intending to keep this "all results are empty" around?
If so uppercase A so doesn't look like a leftover

[hjd, 2017/05/02 12:38:37]

oops, thanks!
Done.

 void ProcessDumpCallbackAdapter(
     GlobalMemoryDumpCallback callback,
@@ -231,6 +230,7 @@ class MemoryDumpManagerTest : public testing::Test {
     last_callback_success_ = false;
     message_loop_.reset(new MessageLoop());
     mdm_.reset(new MemoryDumpManager());
+    results_.clear();
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_.get(), MemoryDumpManager::GetInstance());
   }
@@ -293,13 +293,32 @@ class MemoryDumpManagerTest : public testing::Test {
     return MemoryDumpManager::kMaxConsecutiveFailuresCount;
   }
 
+  const std::vector<MemoryDumpCallbackResult>* GetResults() const {
+    return &results_;
+  }
+
   const MemoryDumpProvider::Options kDefaultOptions;
   std::unique_ptr<MemoryDumpManager> mdm_;
   GlobalMemoryDumpHandler global_dump_handler_;
   bool last_callback_success_;
 
+  // Adapts a ProcessMemoryDumpCallback into a GlobalMemoryDumpCallback by
+  // trimming off the result argument and calling the global callback.

[Primiano Tucci (use gerrit), 2017/04/28 15:04:33]

"trimming off" the result confused me a bit before reading the code.
IIUC the thing here is that the G callback doesn't take a result, but the P does
and you want it.
So I'd probably s/by trimming off the result argument.*$/and keeps around the
process-local result./

[hjd, 2017/05/02 12:38:37]

Done.

+  // all results are empty.
+  void ProcessDumpRecordingCallbackAdapter(
+      GlobalMemoryDumpCallback callback,
+      uint64_t dump_guid,
+      bool success,
+      const base::Optional<MemoryDumpCallbackResult>& result) {
+    if (result.has_value()) {
+      results_.push_back(result.value());
+    }
+    callback.Run(dump_guid, success);
+  }
+
  private:
   std::unique_ptr<MessageLoop> message_loop_;
+  std::vector<MemoryDumpCallbackResult> results_;
 
   // We want our singleton torn down after each test.
   ShadowingAtExitManager at_exit_manager_;
@@ -1282,5 +1301,72 @@ TEST_F(MemoryDumpManagerTest, DumpWithTracingDisabled) {
   mdm_->UnregisterDumpProvider(&mdp);
 }
 
+TEST_F(MemoryDumpManagerTest, TestComputingSummary) {

[Primiano Tucci (use gerrit), 2017/04/28 15:04:33]

s/TestComputingSummary/TestSummaryComputation/

just for consistency with the other test_f above that seem all nouns (% "Test")

[hjd, 2017/05/02 12:38:37]

Done.

+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  MockMemoryDumpProvider mdp;
+  RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
+
+  const MemoryDumpSessionState* session_state =
+      mdm_->session_state_for_testing().get();
+
+  EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _))
+      .WillOnce(Invoke([this](const MemoryDumpRequestArgs& args,
+                              const GlobalMemoryDumpCallback& callback) {
+        ProcessMemoryDumpCallback process_callback =
+            Bind(&MemoryDumpManagerTest_TestComputingSummary_Test::
+                     ProcessDumpRecordingCallbackAdapter,
+                 Unretained(this), callback);
+        mdm_->CreateProcessDump(args, process_callback);
+      }));
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _))
+      .Times(1)
+      .WillRepeatedly(Invoke([session_state](const MemoryDumpArgs&,
+                                             ProcessMemoryDump* pmd) -> bool {
+        auto* size = MemoryAllocatorDump::kNameSize;
+        auto* bytes = MemoryAllocatorDump::kUnitsBytes;
+        auto kb = 1024;

[Primiano Tucci (use gerrit), 2017/04/28 15:04:33]

const uint32_t kB = 1024;
kb -> kB will both match our coding style for constants, and also the actual
unit abbreviation. This make my OCD so happy. 

Also I generally tend to avoid auto unless it causes more wrapping or is
extremely obvious, as it helps readability. (And integers are the less obvious
thing to me in C++, which bite me every other day)

[hjd, 2017/05/02 12:38:37]

Done.

+
+        pmd->CreateAllocatorDump("malloc")->AddScalar(size, bytes, 1 * kb);
+        pmd->CreateAllocatorDump("malloc/foo")->AddScalar(size, bytes, 99 * kb);
+
+        pmd->CreateAllocatorDump("blink_gc")->AddScalar(size, bytes, 2 * kb);
+        pmd->CreateAllocatorDump("blink_gc/foo")
+            ->AddScalar(size, bytes, 99 * kb);
+
+        pmd->CreateAllocatorDump("v8/foo")->AddScalar(size, bytes, 1 * kb);
+        pmd->CreateAllocatorDump("v8/bar")->AddScalar(size, bytes, 2 * kb);
+        pmd->CreateAllocatorDump("v8")->AddScalar(size, bytes, 99 * kb);
+
+        pmd->CreateAllocatorDump("partition_alloc")
+            ->AddScalar(size, bytes, 99 * kb);
+        pmd->CreateAllocatorDump("partition_alloc/partitions")
+            ->AddScalar(size, bytes, 99 * kb);
+        pmd->CreateAllocatorDump("partition_alloc/partitions/foo")
+            ->AddScalar(size, bytes, 2 * kb);
+        pmd->CreateAllocatorDump("partition_alloc/partitions/bar")
+            ->AddScalar(size, bytes, 2 * kb);
+        pmd->process_totals()->set_resident_set_bytes(5 * kb);
+        pmd->set_has_process_totals();
+        return true;
+      }));
+
+  last_callback_success_ = false;
+
+  EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
+  RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                           MemoryDumpLevelOfDetail::LIGHT);
+  DisableTracing();
+

[Primiano Tucci (use gerrit), 2017/04/28 15:04:33]

can you add a comment explaining why the 99 doesn't show up here. 
I guess it will be definitely not obvious for anybody who didn't follow all the
offline conversations.
Another trick I use in this case is to use "malloc/ignored" instead of "foo" in
the strings to make it a bit more obvious (although, I realize while suggesting
this that it will cause you wrapping over 80 cols. You have my compassion)

[hjd, 2017/05/02 12:38:36]

Yeah XD I ended up wrapping some of the others anyway though so I've gone ahead
and updated it now.

+  EXPECT_TRUE(last_callback_success_);
+  ASSERT_EQ(1u, GetResults()->size());
+  MemoryDumpCallbackResult result = GetResults()->front();
+  EXPECT_EQ(1u, result.chrome_dump.malloc_total_kb);
+  EXPECT_EQ(2u, result.chrome_dump.blink_gc_total_kb);
+  EXPECT_EQ(3u, result.chrome_dump.v8_total_kb);
+  EXPECT_EQ(4u, result.chrome_dump.partition_alloc_total_kb);
+  EXPECT_EQ(5u, result.os_dump.resident_set_kb);
+};
+
 }  // namespace trace_event
 }  // namespace base