Removing duplicate code

Summary:
While working on https://reviews.facebook.net/D43179 , I found
duplicate code in the tests. This patch removes it.

Test Plan: make clean all check

Reviewers: igor, sdong, rven, anthony, yhchiang

Reviewed By: yhchiang

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D43263

db/column_family_test.cc

@@ -381,13 +381,6 @@ class ColumnFamilyTest : public testing::Test {
   Random rnd_;
 };
 
-class DumbLogger : public Logger {
- public:
-  using Logger::Logv;
-  virtual void Logv(const char* format, va_list ap) override {}
-  virtual size_t GetLogFileSize() const override { return 0; }
-};
-
 TEST_F(ColumnFamilyTest, DontReuseColumnFamilyID) {
   for (int iter = 0; iter < 3; ++iter) {
     Open();

db/log_test.cc

@@ -45,46 +45,6 @@ static std::string RandomSkewedString(int i, Random* rnd) {
 
 class LogTest : public testing::Test {
  private:
-  class StringDest : public WritableFile {
-   public:
-    std::string contents_;
-
-    explicit StringDest(Slice& reader_contents) :
-      WritableFile(),
-      contents_(""),
-      reader_contents_(reader_contents),
-      last_flush_(0) {
-      reader_contents_ = Slice(contents_.data(), 0);
-    };
-
-    virtual Status Close() override { return Status::OK(); }
-    virtual Status Flush() override {
-      EXPECT_TRUE(reader_contents_.size() <= last_flush_);
-      size_t offset = last_flush_ - reader_contents_.size();
-      reader_contents_ = Slice(
-          contents_.data() + offset,
-          contents_.size() - offset);
-      last_flush_ = contents_.size();
-
-      return Status::OK();
-    }
-    virtual Status Sync() override { return Status::OK(); }
-    virtual Status Append(const Slice& slice) override {
-      contents_.append(slice.data(), slice.size());
-      return Status::OK();
-    }
-    void Drop(size_t bytes) {
-      contents_.resize(contents_.size() - bytes);
-      reader_contents_ = Slice(
-          reader_contents_.data(), reader_contents_.size() - bytes);
-      last_flush_ = contents_.size();
-    }
-
-   private:
-    Slice& reader_contents_;
-    size_t last_flush_;
-  };
-
   class StringSource : public SequentialFile {
    public:
     Slice& contents_;
@@ -165,14 +125,15 @@ class LogTest : public testing::Test {
   };
 
   std::string& dest_contents() {
-    auto dest = dynamic_cast<StringDest*>(writer_.file()->writable_file());
+    auto dest =
+      dynamic_cast<test::StringSink*>(writer_.file()->writable_file());
     assert(dest);
     return dest->contents_;
   }
 
   const std::string& dest_contents() const {
     auto dest =
-        dynamic_cast<const StringDest*>(writer_.file()->writable_file());
+      dynamic_cast<const test::StringSink*>(writer_.file()->writable_file());
     assert(dest);
     return dest->contents_;
   }
@@ -198,7 +159,8 @@ class LogTest : public testing::Test {
   LogTest()
       : reader_contents_(),
         dest_holder_(
-            test::GetWritableFileWriter(new StringDest(reader_contents_))),
+            test::GetWritableFileWriter(
+              new test::StringSink(&reader_contents_))),
         source_holder_(
             test::GetSequentialFileReader(new StringSource(reader_contents_))),
         writer_(std::move(dest_holder_)),
@@ -232,7 +194,8 @@ class LogTest : public testing::Test {
   }
 
   void ShrinkSize(int bytes) {
-    auto dest = dynamic_cast<StringDest*>(writer_.file()->writable_file());
+    auto dest =
+      dynamic_cast<test::StringSink*>(writer_.file()->writable_file());
     assert(dest);
     dest->Drop(bytes);
   }

db/memtable_list_test.cc

@@ -13,18 +13,12 @@
 #include "db/writebuffer.h"
 #include "rocksdb/db.h"
 #include "rocksdb/status.h"
+#include "util/testutil.h"
 #include "util/string_util.h"
 #include "util/testharness.h"
 
 namespace rocksdb {
 
-class DumbLogger : public Logger {
- public:
-  using Logger::Logv;
-  virtual void Logv(const char* format, va_list ap) override {}
-  virtual size_t GetLogFileSize() const override { return 0; }
-};
-
 class MemTableListTest : public testing::Test {
  public:
   std::string dbname;
@@ -58,7 +52,7 @@ class MemTableListTest : public testing::Test {
       MemTableList* list, const MutableCFOptions& mutable_cf_options,
       const autovector<MemTable*>& m, autovector<MemTable*>* to_delete) {
     // Create a mock Logger
-    DumbLogger logger;
+    test::NullLogger logger;
     LogBuffer log_buffer(DEBUG_LEVEL, &logger);
 
     // Create a mock VersionSet

db/table_properties_collector_test.cc

@@ -32,65 +32,6 @@ class TablePropertiesTest : public testing::Test,
   bool backward_mode_;
 };
 
-// TODO(kailiu) the following classes should be moved to some more general
-// places, so that other tests can also make use of them.
-// `FakeWritableFileWriter* file system
-// and therefore enable us to quickly setup the tests.
-class FakeWritableFile : public WritableFile {
- public:
-  ~FakeWritableFile() { }
-
-  const std::string& contents() const { return contents_; }
-
-  virtual Status Close() override { return Status::OK(); }
-  virtual Status Flush() override { return Status::OK(); }
-  virtual Status Sync() override { return Status::OK(); }
-
-  virtual Status Append(const Slice& data) override {
-    contents_.append(data.data(), data.size());
-    return Status::OK();
-  }
-
- private:
-  std::string contents_;
-};
-
-
-class FakeRandomeAccessFile : public RandomAccessFile {
- public:
-  explicit FakeRandomeAccessFile(const Slice& contents)
-      : contents_(contents.data(), contents.size()) {
-  }
-
-  virtual ~FakeRandomeAccessFile() { }
-
-  uint64_t Size() const { return contents_.size(); }
-
-  virtual Status Read(uint64_t offset, size_t n, Slice* result,
-                      char* scratch) const override {
-    if (offset > contents_.size()) {
-      return Status::InvalidArgument("invalid Read offset");
-    }
-    if (offset + n > contents_.size()) {
-      n = contents_.size() - offset;
-    }
-    memcpy(scratch, &contents_[offset], n);
-    *result = Slice(scratch, n);
-    return Status::OK();
-  }
-
- private:
-  std::string contents_;
-};
-
-
-class DumbLogger : public Logger {
- public:
-  using Logger::Logv;
-  virtual void Logv(const char* format, va_list ap) override {}
-  virtual size_t GetLogFileSize() const override { return 0; }
-};
-
 // Utilities test functions
 namespace {
 void MakeBuilder(const Options& options, const ImmutableCFOptions& ioptions,
@@ -99,7 +40,7 @@ void MakeBuilder(const Options& options, const ImmutableCFOptions& ioptions,
                      int_tbl_prop_collector_factories,
                  std::unique_ptr<WritableFileWriter>* writable,
                  std::unique_ptr<TableBuilder>* builder) {
-  unique_ptr<WritableFile> wf(new FakeWritableFile);
+  unique_ptr<WritableFile> wf(new test::StringSink);
   writable->reset(new WritableFileWriter(std::move(wf), EnvOptions()));
 
   builder->reset(NewTableBuilder(
@@ -316,11 +257,11 @@ void TestCustomizedTablePropertiesCollector(
   writer->Flush();
 
   // -- Step 2: Read properties
-  FakeWritableFile* fwf =
+  test::StringSink* fwf =
-      static_cast<FakeWritableFile*>(writer->writable_file());
+      static_cast<test::StringSink*>(writer->writable_file());
   std::unique_ptr<RandomAccessFileReader> fake_file_reader(
       test::GetRandomAccessFileReader(
-          new FakeRandomeAccessFile(fwf->contents())));
+          new test::StringSource(fwf->contents())));
   TableProperties* props;
   Status s = ReadTableProperties(fake_file_reader.get(), fwf->contents().size(),
                                  magic_number, Env::Default(), nullptr, &props);
@@ -427,7 +368,7 @@ void TestInternalKeyPropertiesCollector(
     auto comparator = options.comparator;
     // HACK: Set options.info_log to avoid writing log in
     // SanitizeOptions().
-    options.info_log = std::make_shared<DumbLogger>();
+    options.info_log = std::make_shared<test::NullLogger>();
     options = SanitizeOptions("db",            // just a place holder
                               &pikc,
                               options);
@@ -449,10 +390,10 @@ void TestInternalKeyPropertiesCollector(
     ASSERT_OK(builder->Finish());
     writable->Flush();
 
-    FakeWritableFile* fwf =
+    test::StringSink* fwf =
-        static_cast<FakeWritableFile*>(writable->writable_file());
+        static_cast<test::StringSink*>(writable->writable_file());
     unique_ptr<RandomAccessFileReader> reader(test::GetRandomAccessFileReader(
-        new FakeRandomeAccessFile(fwf->contents())));
+        new test::StringSource(fwf->contents())));
     TableProperties* props;
     Status s =
         ReadTableProperties(reader.get(), fwf->contents().size(), magic_number,

include/rocksdb/table_properties.h

@@ -87,10 +87,10 @@ enum EntryType {
 };
 
 // `TablePropertiesCollector` provides the mechanism for users to collect
-// their own interested properties. This class is essentially a collection
+// their own properties that they are interested in. This class is essentially
-// of callback functions that will be invoked during table building.
+// a collection of callback functions that will be invoked during table
-// It is construced with TablePropertiesCollectorFactory. The methods don't
+// building. It is construced with TablePropertiesCollectorFactory. The methods
-// need to be thread-safe, as we will create exactly one
+// don't need to be thread-safe, as we will create exactly one
 // TablePropertiesCollector object per table and then call it sequentially
 class TablePropertiesCollector {
  public:
@@ -114,7 +114,7 @@ class TablePropertiesCollector {
   virtual Status AddUserKey(const Slice& key, const Slice& value,
                             EntryType type, SequenceNumber seq,
                             uint64_t file_size) {
-    // For backward-compatible.
+    // For backwards-compatibility.
     return Add(key, value);
   }
 

table/table_test.cc

@@ -119,71 +119,6 @@ struct STLLessThan {
 
 }  // namespace
 
-class StringSink: public WritableFile {
- public:
-  ~StringSink() { }
-
-  const std::string& contents() const { return contents_; }
-
-  virtual Status Close() override { return Status::OK(); }
-  virtual Status Flush() override { return Status::OK(); }
-  virtual Status Sync() override { return Status::OK(); }
-
-  virtual Status Append(const Slice& data) override {
-    contents_.append(data.data(), data.size());
-    return Status::OK();
-  }
-
- private:
-  std::string contents_;
-};
-
-
-class StringSource: public RandomAccessFile {
- public:
-  StringSource(const Slice& contents, uint64_t uniq_id, bool mmap)
-      : contents_(contents.data(), contents.size()), uniq_id_(uniq_id),
-        mmap_(mmap) {
-  }
-
-  virtual ~StringSource() { }
-
-  uint64_t Size() const { return contents_.size(); }
-
-  virtual Status Read(uint64_t offset, size_t n, Slice* result,
-                      char* scratch) const override {
-    if (offset > contents_.size()) {
-      return Status::InvalidArgument("invalid Read offset");
-    }
-    if (offset + n > contents_.size()) {
-      n = contents_.size() - offset;
-    }
-    if (!mmap_) {
-      memcpy(scratch, &contents_[offset], n);
-      *result = Slice(scratch, n);
-    } else {
-      *result = Slice(&contents_[offset], n);
-    }
-    return Status::OK();
-  }
-
-  virtual size_t GetUniqueId(char* id, size_t max_size) const override {
-    if (max_size < 20) {
-      return 0;
-    }
-
-    char* rid = id;
-    rid = EncodeVarint64(rid, uniq_id_);
-    rid = EncodeVarint64(rid, 0);
-    return static_cast<size_t>(rid-id);
-  }
-
- private:
-  std::string contents_;
-  uint64_t uniq_id_;
-  bool mmap_;
-};
-
 typedef std::map<std::string, std::string, STLLessThan> KVMap;
 
 // Helper class for tests to unify the interface between
@@ -347,7 +282,7 @@ class TableConstructor: public Constructor {
                             const InternalKeyComparator& internal_comparator,
                             const KVMap& kv_map) override {
     Reset();
-    file_writer_.reset(test::GetWritableFileWriter(new StringSink()));
+    file_writer_.reset(test::GetWritableFileWriter(new test::StringSink()));
     unique_ptr<TableBuilder> builder;
     std::vector<std::unique_ptr<IntTblPropCollectorFactory>>
         int_tbl_prop_collector_factories;
@@ -376,7 +311,7 @@ class TableConstructor: public Constructor {
 
     // Open the table
     uniq_id_ = cur_uniq_id_++;
-    file_reader_.reset(test::GetRandomAccessFileReader(new StringSource(
+    file_reader_.reset(test::GetRandomAccessFileReader(new test::StringSource(
         GetSink()->contents(), uniq_id_, ioptions.allow_mmap_reads)));
     return ioptions.table_factory->NewTableReader(
         ioptions, soptions, internal_comparator, std::move(file_reader_),
@@ -398,7 +333,7 @@ class TableConstructor: public Constructor {
   }
 
   virtual Status Reopen(const ImmutableCFOptions& ioptions) {
-    file_reader_.reset(test::GetRandomAccessFileReader(new StringSource(
+    file_reader_.reset(test::GetRandomAccessFileReader(new test::StringSource(
         GetSink()->contents(), uniq_id_, ioptions.allow_mmap_reads)));
     return ioptions.table_factory->NewTableReader(
         ioptions, soptions, *last_internal_key_, std::move(file_reader_),
@@ -421,8 +356,8 @@ class TableConstructor: public Constructor {
     file_reader_.reset();
   }
 
-  StringSink* GetSink() {
+  test::StringSink* GetSink() {
-    return static_cast<StringSink*>(file_writer_->writable_file());
+    return static_cast<test::StringSink*>(file_writer_->writable_file());
   }
 
   uint64_t uniq_id_;
@@ -1782,9 +1717,9 @@ TEST_F(PlainTableTest, BasicPlainTableProperties) {
   plain_table_options.hash_table_ratio = 0;
 
   PlainTableFactory factory(plain_table_options);
-  StringSink sink;
+  test::StringSink sink;
   unique_ptr<WritableFileWriter> file_writer(
-      test::GetWritableFileWriter(new StringSink()));
+      test::GetWritableFileWriter(new test::StringSink()));
   Options options;
   const ImmutableCFOptions ioptions(options);
   InternalKeyComparator ikc(options.comparator);
@@ -1804,10 +1739,11 @@ TEST_F(PlainTableTest, BasicPlainTableProperties) {
   ASSERT_OK(builder->Finish());
   file_writer->Flush();
 
-  StringSink* ss = static_cast<StringSink*>(file_writer->writable_file());
+  test::StringSink* ss =
+    static_cast<test::StringSink*>(file_writer->writable_file());
   unique_ptr<RandomAccessFileReader> file_reader(
       test::GetRandomAccessFileReader(
-          new StringSource(ss->contents(), 72242, true)));
+          new test::StringSource(ss->contents(), 72242, true)));
 
   TableProperties* props = nullptr;
   auto s = ReadTableProperties(file_reader.get(), ss->contents().size(),

util/testutil.h

@@ -168,5 +168,105 @@ extern RandomAccessFileReader* GetRandomAccessFileReader(RandomAccessFile* raf);
 
 extern SequentialFileReader* GetSequentialFileReader(SequentialFile* se);
 
+class StringSink: public WritableFile {
+ public:
+  std::string contents_;
+
+  explicit StringSink(Slice* reader_contents = nullptr) :
+      WritableFile(),
+      contents_(""),
+      reader_contents_(reader_contents),
+      last_flush_(0) {
+    if (reader_contents_ != nullptr) {
+      *reader_contents_ = Slice(contents_.data(), 0);
+    }
+  }
+
+  const std::string& contents() const { return contents_; }
+
+  virtual Status Close() override { return Status::OK(); }
+  virtual Status Flush() override {
+    if (reader_contents_ != nullptr) {
+      assert(reader_contents_->size() <= last_flush_);
+      size_t offset = last_flush_ - reader_contents_->size();
+      *reader_contents_ = Slice(
+          contents_.data() + offset,
+          contents_.size() - offset);
+      last_flush_ = contents_.size();
+    }
+
+    return Status::OK();
+  }
+  virtual Status Sync() override { return Status::OK(); }
+  virtual Status Append(const Slice& slice) override {
+    contents_.append(slice.data(), slice.size());
+    return Status::OK();
+  }
+  void Drop(size_t bytes) {
+    if (reader_contents_ != nullptr) {
+      contents_.resize(contents_.size() - bytes);
+      *reader_contents_ = Slice(
+          reader_contents_->data(), reader_contents_->size() - bytes);
+      last_flush_ = contents_.size();
+    }
+  }
+
+ private:
+  Slice* reader_contents_;
+  size_t last_flush_;
+};
+
+class StringSource: public RandomAccessFile {
+ public:
+  StringSource(const Slice& contents, uint64_t uniq_id = 0, bool mmap = false)
+    : contents_(contents.data(), contents.size()), uniq_id_(uniq_id),
+    mmap_(mmap) {
+    }
+
+  virtual ~StringSource() { }
+
+  uint64_t Size() const { return contents_.size(); }
+
+  virtual Status Read(uint64_t offset, size_t n, Slice* result,
+      char* scratch) const override {
+    if (offset > contents_.size()) {
+      return Status::InvalidArgument("invalid Read offset");
+    }
+    if (offset + n > contents_.size()) {
+      n = contents_.size() - offset;
+    }
+    if (!mmap_) {
+      memcpy(scratch, &contents_[offset], n);
+      *result = Slice(scratch, n);
+    } else {
+      *result = Slice(&contents_[offset], n);
+    }
+    return Status::OK();
+  }
+
+  virtual size_t GetUniqueId(char* id, size_t max_size) const override {
+    if (max_size < 20) {
+      return 0;
+    }
+
+    char* rid = id;
+    rid = EncodeVarint64(rid, uniq_id_);
+    rid = EncodeVarint64(rid, 0);
+    return static_cast<size_t>(rid-id);
+  }
+
+ private:
+  std::string contents_;
+  uint64_t uniq_id_;
+  bool mmap_;
+};
+
+class NullLogger : public Logger {
+ public:
+  using Logger::Logv;
+  virtual void Logv(const char* format, va_list ap) override {}
+  virtual size_t GetLogFileSize() const override { return 0; }
+};
+
 }  // namespace test
 }  // namespace rocksdb