[Chromecast] Move SynchronizedMinidumpManager ratelimit logic to child classes.

chromecast/crash/linux/synchronized_minidump_manager_unittest.cc

Index: chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
diff --git a/chromecast/crash/linux/synchronized_minidump_manager_unittest.cc b/chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
index 94dabd611d9088a795f50a8740ca68ca71c3298b..d8a7f3e75dd188113e2eec7ab2dfe9de0e3eb54e 100644
--- a/chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
+++ b/chromecast/crash/linux/synchronized_minidump_manager_unittest.cc
@@ -35,8 +35,8 @@ const char kMetadataName[] = "metadata";
 const char kMinidumpSubdir[] = "minidumps";
 
 // A trivial implementation of SynchronizedMinidumpManager, which does no work
-// to the
-// minidump and exposes its protected members for testing.
+// to the minidump and exposes its protected members for testing. This simply
+// adds an entry to the lockfile.
 class SynchronizedMinidumpManagerSimple : public SynchronizedMinidumpManager {
  public:
   SynchronizedMinidumpManagerSimple()
@@ -77,6 +77,41 @@ void DoWorkLockedTask(SynchronizedMinidumpManagerSimple* manager) {
   manager->DoWorkLocked();
 }
 
+// Simple SynchronizedMinidumpManager consumer. Checks if a dump can be uploaded
+// then removes it from the lockfile.
+class SynchronizedMinidumpManagerUploader : public SynchronizedMinidumpManager {

[slan, 2015/09/09 19:08:11]

Please rename to SynchronizedMinidumpManagerConsumer, Fake*Uploader, or
something similar.

[bcf_google, 2015/09/09 21:20:29]

Done.

+ public:
+  SynchronizedMinidumpManagerUploader()
+      : SynchronizedMinidumpManager(),
+        can_upload_return_val_(false),
+        lockfile_path_(dump_path_.Append(kLockfileName).value()) {}
+  ~SynchronizedMinidumpManagerUploader() override {}
+
+  int DoWorkLocked() { return AcquireLockAndDoWork(); }
+
+  // SynchronizedMinidumpManager implementation:
+  int DoWork() override {
+    can_upload_return_val_ = CanUploadDump();
+
+    if (RemoveEntryFromLockFile(0) < 0)
+      return -1;
+
+    if (IncrementRatelimitPeriodDumps() < 0)
+      return -1;
+
+    return 0;
+  }
+
+  // Accessors for testing.
+  const std::string& dump_path() { return dump_path_.value(); }

[slan, 2015/09/09 19:08:11]

I don't see these methods called?

[bcf_google, 2015/09/09 21:20:29]

Okay they were removed.

+  const std::string& lockfile_path() { return lockfile_path_; }
+  bool can_upload_return_val() { return can_upload_return_val_; }
+
+ private:
+  bool can_upload_return_val_;
+  std::string lockfile_path_;
+};
+
 class SleepySynchronizedMinidumpManagerSimple
     : public SynchronizedMinidumpManagerSimple {
  public:
@@ -384,37 +419,15 @@ TEST_F(SynchronizedMinidumpManagerTest,
 }
 
 TEST_F(SynchronizedMinidumpManagerTest,

[slan, 2015/09/09 19:08:11]

I know that this test was like this before this change, but it seems a little
too complex. Currently it tests 3 bits of functionality:
 * Upload is successful when limit not exceeded.
 * Upload is not successful when limit exceeded.
 * Upload is successful when period rolls over. 

Can this be split into 3 separate unit tests (without being painfully redundant
when testing the third bullet :-) )?

[bcf_google, 2015/09/09 21:20:29]

How does this look?

-       AddEntryFailsWhenTooManyRecentDumpsPresent) {
+       CanUploadFailsWhenRatelimitPeriodExceeded) {
   // Sample parameters.
   time_t now = time(0);
   MinidumpParams params;
   params.process_name = "process";
 
-  SynchronizedMinidumpManagerSimple manager;
-  manager.SetDumpInfoToWrite(
-      make_scoped_ptr(new DumpInfo("dump1", "log1", now, params)));
-
-  for (int i = 0; i < SynchronizedMinidumpManager::kMaxLockfileDumps; ++i) {
-    // Adding these should succeed
-    ASSERT_EQ(0, manager.DoWorkLocked());
-    ASSERT_EQ(0, manager.add_entry_return_code());
-  }
-
-  ASSERT_EQ(0, manager.DoWorkLocked());
-
-  // This one should fail
-  ASSERT_GT(0, manager.add_entry_return_code());
-}
-
-TEST_F(SynchronizedMinidumpManagerTest,
-       AddEntryFailsWhenRatelimitPeriodExceeded) {
-  // Sample parameters.
-  time_t now = time(0);
-  MinidumpParams params;
-  params.process_name = "process";
-
-  SynchronizedMinidumpManagerSimple manager;
-  manager.SetDumpInfoToWrite(
+  SynchronizedMinidumpManagerSimple producer;
+  SynchronizedMinidumpManagerUploader uploader;
+  producer.SetDumpInfoToWrite(
       make_scoped_ptr(new DumpInfo("dump1", "log1", now, params)));
 
   // Multiple iters to make sure period resets work correctly
@@ -425,31 +438,40 @@ TEST_F(SynchronizedMinidumpManagerTest,
     size_t too_many_recent_dumps =
         SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps;
     for (size_t i = 0; i < too_many_recent_dumps; ++i) {
-      // Adding these should succeed
-      ASSERT_EQ(0, manager.DoWorkLocked());
-      ASSERT_EQ(0, manager.add_entry_return_code());
+      // Uploading these should succeed
+      ASSERT_EQ(0, producer.DoWorkLocked());
+      ASSERT_EQ(0, producer.add_entry_return_code());
 
-      // Clear dumps so we don't reach max dumps in lockfile
-      ASSERT_TRUE(ClearDumps(lockfile_.value()));
+      ASSERT_EQ(0, uploader.DoWorkLocked());
+      ASSERT_EQ(true, uploader.can_upload_return_val());
     }
 
-    ASSERT_EQ(0, manager.DoWorkLocked());
+    ASSERT_EQ(0, producer.DoWorkLocked());
+    ASSERT_EQ(0, producer.add_entry_return_code());
+
     // Should fail with too many dumps
-    ASSERT_GT(0, manager.add_entry_return_code());
+    ASSERT_EQ(0, uploader.DoWorkLocked());
+    ASSERT_EQ(false, uploader.can_upload_return_val());
 
     int64 period = SynchronizedMinidumpManager::kRatelimitPeriodSeconds;
 
+    ASSERT_EQ(0, producer.DoWorkLocked());
+    ASSERT_EQ(0, producer.add_entry_return_code());
+
     // Half period shouldn't trigger reset
     SetRatelimitPeriodStart(metadata_.value(), now - period / 2);
-    ASSERT_EQ(0, manager.DoWorkLocked());
-    ASSERT_GT(0, manager.add_entry_return_code());
+    ASSERT_EQ(0, uploader.DoWorkLocked());
+    ASSERT_EQ(false, uploader.can_upload_return_val());
 
     // Set period starting time to trigger a reset
     SetRatelimitPeriodStart(metadata_.value(), now - period);
   }
 
-  ASSERT_EQ(0, manager.DoWorkLocked());
-  ASSERT_EQ(0, manager.add_entry_return_code());
+  ASSERT_EQ(0, producer.DoWorkLocked());
+  ASSERT_EQ(0, producer.add_entry_return_code());
+
+  ASSERT_EQ(0, uploader.DoWorkLocked());
+  ASSERT_EQ(true, uploader.can_upload_return_val());
 }
 
 }  // namespace chromecast