memory-infra: Add unittest for MemoryDumpManager summary calculations

base/trace_event/memory_dump_manager_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 "base/trace_event/memory_dump_manager.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
@@ skipping 100 matching lines @@
   task_runner->PostTask(from_here, std::move(task));
   task_runner->PostTask(FROM_HERE, base::BindOnce(&WaitableEvent::Signal,
                                                   base::Unretained(&event)));
   // The SequencedTaskRunner guarantees that |event| will only be signaled after
   // |task| is executed.
   event.Wait();
 }
 
 // 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,
     uint64_t dump_guid,
     bool success,
     const base::Optional<base::trace_event::MemoryDumpCallbackResult>&) {
   callback.Run(dump_guid, success);
 }
 
 // This mocks the RequestGlobalDumpFunction which is typically handled by
 // process_local_dump_manager_impl.cc, by short-circuiting dump requests locally
@@ skipping 91 matching lines @@
 }  // namespace
 
 class MemoryDumpManagerTest : public testing::Test {
  public:
   MemoryDumpManagerTest() : testing::Test(), kDefaultOptions() {}
 
   void SetUp() override {
     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());
   }
 
   void TearDown() override {
     MemoryDumpManager::SetInstanceForTesting(nullptr);
     mdm_.reset();
     message_loop_.reset();
     TraceLog::DeleteForTesting();
   }
@@ skipping 42 matching lines @@
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
   bool IsPeriodicDumpingEnabled() const {
     return MemoryDumpScheduler::GetInstance()->is_enabled_for_testing();
   }
 
   int GetMaxConsecutiveFailuresCount() const {
     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_;
 };
 
 // Basic sanity checks. Registers a memory dump provider and checks that it is
 // called, but only when memory-infra is enabled.
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
   MockMemoryDumpProvider mdp;
@@ skipping 962 matching lines @@
   // The callback result should actually be false since (for the moment at
   // least) a true result means that as well as the dump generally being
   // successful we also managed to add the dump to the trace.
   EXPECT_FALSE(last_callback_success_);
 
   mdm_->Disable();
 
   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