From c4650710294e3b4a57361a7df7c397e042d051ec Mon Sep 17 00:00:00 2001 From: Andres Notzli Date: Wed, 5 Aug 2015 07:33:27 -0700 Subject: [PATCH] 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 | 7 -- db/log_test.cc | 51 ++----------- db/memtable_list_test.cc | 10 +-- db/table_properties_collector_test.cc | 75 +++---------------- include/rocksdb/table_properties.h | 10 +-- table/table_test.cc | 84 +++------------------- util/testutil.h | 100 ++++++++++++++++++++++++++ 7 files changed, 132 insertions(+), 205 deletions(-) diff --git a/db/column_family_test.cc b/db/column_family_test.cc index 86294b003b..88882f3b03 100644 --- a/db/column_family_test.cc +++ b/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(); diff --git a/db/log_test.cc b/db/log_test.cc index 168936e98f..5ab41f2510 100644 --- a/db/log_test.cc +++ b/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(writer_.file()->writable_file()); + auto dest = + dynamic_cast(writer_.file()->writable_file()); assert(dest); return dest->contents_; } const std::string& dest_contents() const { auto dest = - dynamic_cast(writer_.file()->writable_file()); + dynamic_cast(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(writer_.file()->writable_file()); + auto dest = + dynamic_cast(writer_.file()->writable_file()); assert(dest); dest->Drop(bytes); } diff --git a/db/memtable_list_test.cc b/db/memtable_list_test.cc index d809d0f165..7bb8b3b21a 100644 --- a/db/memtable_list_test.cc +++ b/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& m, autovector* to_delete) { // Create a mock Logger - DumbLogger logger; + test::NullLogger logger; LogBuffer log_buffer(DEBUG_LEVEL, &logger); // Create a mock VersionSet diff --git a/db/table_properties_collector_test.cc b/db/table_properties_collector_test.cc index 4ac9f2619d..403de6e25a 100644 --- a/db/table_properties_collector_test.cc +++ b/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* writable, std::unique_ptr* builder) { - unique_ptr wf(new FakeWritableFile); + unique_ptr 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 = - static_cast(writer->writable_file()); + test::StringSink* fwf = + static_cast(writer->writable_file()); std::unique_ptr 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(); + options.info_log = std::make_shared(); options = SanitizeOptions("db", // just a place holder &pikc, options); @@ -449,10 +390,10 @@ void TestInternalKeyPropertiesCollector( ASSERT_OK(builder->Finish()); writable->Flush(); - FakeWritableFile* fwf = - static_cast(writable->writable_file()); + test::StringSink* fwf = + static_cast(writable->writable_file()); unique_ptr reader(test::GetRandomAccessFileReader( - new FakeRandomeAccessFile(fwf->contents()))); + new test::StringSource(fwf->contents()))); TableProperties* props; Status s = ReadTableProperties(reader.get(), fwf->contents().size(), magic_number, diff --git a/include/rocksdb/table_properties.h b/include/rocksdb/table_properties.h index 892cfb26e1..5a4096d010 100644 --- a/include/rocksdb/table_properties.h +++ b/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 -// of callback functions that will be invoked during table building. -// It is construced with TablePropertiesCollectorFactory. The methods don't -// need to be thread-safe, as we will create exactly one +// their own properties that they are interested in. This class is essentially +// a collection of callback functions that will be invoked during table +// building. It is construced with TablePropertiesCollectorFactory. The methods +// 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); } diff --git a/table/table_test.cc b/table/table_test.cc index cf8c0d0683..6fd201b83c 100644 --- a/table/table_test.cc +++ b/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(rid-id); - } - - private: - std::string contents_; - uint64_t uniq_id_; - bool mmap_; -}; - typedef std::map 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 builder; std::vector> 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() { - return static_cast(file_writer_->writable_file()); + test::StringSink* GetSink() { + return static_cast(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 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(file_writer->writable_file()); + test::StringSink* ss = + static_cast(file_writer->writable_file()); unique_ptr 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(), diff --git a/util/testutil.h b/util/testutil.h index 24eceddfd2..c02c4151b0 100644 --- a/util/testutil.h +++ b/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(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