[Chromecast] Move SynchronizedMinidumpManager ratelimit logic to child classes.

chromecast/crash/linux/synchronized_minidump_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 <fcntl.h>
 #include <stdlib.h>
 #include <sys/file.h>
 #include <sys/stat.h>   // mkdir
 #include <sys/types.h>  //
 #include <stdio.h>      // perror
@@ skipping 17 matching lines @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromecast {
 namespace {
 
 const char kLockfileName[] = "lockfile";
 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()
       : SynchronizedMinidumpManager(),
         work_done_(false),
         add_entry_return_code_(-1),
         lockfile_path_(dump_path_.Append(kLockfileName).value()) {}
   ~SynchronizedMinidumpManagerSimple() override {}
 
   void SetDumpInfoToWrite(scoped_ptr<DumpInfo> dump_info) {
@@ skipping 20 matching lines @@
   bool work_done_;
   int add_entry_return_code_;
   std::string lockfile_path_;
   scoped_ptr<DumpInfo> dump_info_;
 };
 
 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:
   SleepySynchronizedMinidumpManagerSimple(int sleep_duration_ms)
       : SynchronizedMinidumpManagerSimple(),
         sleep_duration_ms_(sleep_duration_ms) {}
   ~SleepySynchronizedMinidumpManagerSimple() override {}
 
   // SynchronizedMinidumpManager implementation:
   int DoWork() override {
@@ skipping 286 matching lines @@
   EXPECT_EQ(0, manager.DoWorkLocked());
   EXPECT_EQ(0, manager.add_entry_return_code());
   EXPECT_TRUE(manager.work_done());
 
   // Test that both entries were logged.
   ScopedVector<DumpInfo> dumps;
   ASSERT_TRUE(FetchDumps(lockfile_.value(), &dumps));
   EXPECT_EQ(2u, dumps.size());
 }
 
 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
   for (int iter = 0; iter < 3; ++iter) {
     time_t now = time(nullptr);
 
     // Write dump logs to the lockfile.
     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