[Tracing] Disable registration of regular dump providers during tests

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 6fb8c321e5bd978b50c6cf1468a684bd14e70ddd..8183a1d203224ec2ec542c9fc89efe0a9592c524 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -52,8 +52,9 @@ class MemoryDumpManagerTest : public testing::Test {
     mdm_.reset(new MemoryDumpManager());
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_, MemoryDumpManager::GetInstance());
-    MemoryDumpManager::GetInstance()->Initialize();
-    MemoryDumpManager::GetInstance()->SetDelegate(&delegate_);
+    mdm_->set_ignore_dumper_registrations_for_testing(true);
+    mdm_->Initialize();
+    mdm_->SetDelegate(&delegate_);

[picksi, 2015/09/22 10:53:38]

Random drive-by thought. For other users (i.e. outside of these tests) does the
order of these calls need to be maintained for correct execution? If so can this
be enforced somehow (at compile time), e.g. Add and InitialzizeWithDelegate()
function or make Initilize return a delegate-handler (?) that is used to set a
delegate... etc.

[Ruud van Asseldonk, 2015/09/29 09:12:27]

The |SetDelegate| method did not survive Primiano’s Great Refactor™. In fact,
the new |Initialize| method takes a delegate.

   }
 
   void TearDown() override {
@@ -75,12 +76,24 @@ class MemoryDumpManagerTest : public testing::Test {
   const char* kTraceCategory = MemoryDumpManager::kTraceCategoryForTesting;
 
   void EnableTracing(const char* category) {
+    bool registrations_ignored =
+        mdm_->ignore_dumper_registrations_for_testing();

[picksi, 2015/09/22 10:53:38]

Nit: This function name makes it sound like you are ignoring dumper
registration, rather than getting it. Should this be renamed to something like
are_dumper_registrations_ignored()?

[Ruud van Asseldonk, 2015/09/29 09:12:27]

Not according to the style guide (unless I also rename the variable) which says:
Do you think the variable should be renamed?

[picksi, 2015/09/29 10:55:56]

You could rename the variable 'dumper_registrations_ignored_for_testing' which
would be less confusing and is in line with your local variable
'registrations_ignored'.

FWIW Are you also able to drop 'dumper_' as a prefix? (I think) the context of
the call, e.g. mdm->registrations_ignored_for_testing(), makes it obvious that
it's a dumper. Then you'd end up with 'registrations_ignored_for_testing'. WDYT?

+    mdm_->set_ignore_dumper_registrations_for_testing(true);
     TraceLog::GetInstance()->SetEnabled(
         TraceConfig(category, ""), TraceLog::RECORDING_MODE);
+    mdm_->set_ignore_dumper_registrations_for_testing(registrations_ignored);
   }
 
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
+  void EnableMemoryDumpProviderRegistrations() {
+    mdm_->set_ignore_dumper_registrations_for_testing(false);
+  }
+
+  void DisableMemoryDumpProviderRegistrations() {

[picksi, 2015/09/22 10:53:38]

nit: Should you be using this function in EnableTracing() instead of directly
calling the setter?

[Ruud van Asseldonk, 2015/09/29 09:12:27]

In |UnregisterDumperFromThreadWhileDumping|, registrations need to be
enabled/disabled at a different point, so a method is necessary either way.

+    mdm_->set_ignore_dumper_registrations_for_testing(true);
+  }
+
   scoped_ptr<MemoryDumpManager> mdm_;
   bool last_callback_success_;
 
@@ -166,6 +179,7 @@ class MockDumpProvider : public MemoryDumpProvider {
 
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
   MockDumpProvider mdp;
+  EnableMemoryDumpProviderRegistrations();

[Primiano Tucci (use gerrit), 2015/09/22 08:07:02]

Hmm this is a unittest. Why do you need all these enable/disable registrations
here? Nothing else should happen (as opposite to a browser test, where random
things can register and unregister)

[Ruud van Asseldonk, 2015/09/29 09:12:27]

I removed |skip_core_dumpers_auto_registration_for_testing_| in favour of
|ignore_dumper_registrations_for_testing_|, so this is required to prevent the
core dumpers from registering.

   mdm_->RegisterDumpProvider(&mdp);
 
   // Check that the dumper is not called if the memory category is not enabled.
@@ -198,6 +212,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
   // Check that requesting dumps with high level of detail actually propagates
   // to OnMemoryDump() call on dump providers.
   MockDumpProvider mdp_high_detail(MemoryDumpArgs::LevelOfDetail::HIGH);
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp_high_detail);
 
   EnableTracing(kTraceCategory);
@@ -231,6 +246,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
 TEST_F(MemoryDumpManagerTest, SharedSessionState) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp1);
   mdm_->RegisterDumpProvider(&mdp2);
 
@@ -256,6 +272,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   MockDumpProvider mdp2;
 
   // Enable only mdp1.
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
@@ -288,6 +305,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
 // registration state and not on previous registrations and dumps.
 TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
   MockDumpProvider mdp;
+  EnableMemoryDumpProviderRegistrations();
 
   mdm_->RegisterDumpProvider(&mdp);
 
@@ -343,6 +361,8 @@ TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
   ScopedVector<Thread> threads;
   ScopedVector<MockDumpProvider> mdps;
 
+  EnableMemoryDumpProviderRegistrations();
+
   // Create the threads and setup the expectations. Given that at each iteration
   // we will pop out one thread/MemoryDumpProvider, each MDP is supposed to be
   // invoked a number of times equal to its index.
@@ -360,6 +380,8 @@ TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
 
   EnableTracing(kTraceCategory);
 
+  DisableMemoryDumpProviderRegistrations();
+
   while (!threads.empty()) {
     last_callback_success_ = false;
     {
@@ -401,6 +423,7 @@ TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
 
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp1);
   mdm_->RegisterDumpProvider(&mdp2);
   EnableTracing(kTraceCategory);
@@ -428,6 +451,7 @@ TEST_F(MemoryDumpManagerTest, RegisterDumperWhileDumping) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
 
+  EnableMemoryDumpProviderRegistrations();
   mdp1.dump_provider_to_register_or_unregister = &mdp2;
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(kTraceCategory);
@@ -458,6 +482,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileDumping) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
 
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp1, ThreadTaskRunnerHandle::Get());
   mdm_->RegisterDumpProvider(&mdp2, ThreadTaskRunnerHandle::Get());
   mdp1.dump_provider_to_register_or_unregister = &mdp2;
@@ -493,7 +518,9 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
   for (int i = 0; i < 2; i++) {
     threads.push_back(new TestIOThread(TestIOThread::kAutoStart));
     mdps.push_back(new MockDumpProvider(threads.back()->task_runner()));
+    EnableMemoryDumpProviderRegistrations();
     mdm_->RegisterDumpProvider(mdps.back(), threads.back()->task_runner());
+    DisableMemoryDumpProviderRegistrations();
   }
 
   int on_memory_dump_call_count = 0;
@@ -544,6 +571,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
 TEST_F(MemoryDumpManagerTest, CallbackCalledOnFailure) {
   MockDumpProvider mdp1;
 
+  EnableMemoryDumpProviderRegistrations();
   mdm_->RegisterDumpProvider(&mdp1);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(0);