Skip to content

Commit

Permalink
Make env*_test work with ASSERT_STATUS_CHECKED (#7176)
Browse files Browse the repository at this point in the history
Summary:
Make (most of) the env*_test pass when ASSERT_STATUS_CHECKED is enabled.

One test that opens a database is currently disabled in this mode, as there are many errors that need revisited for DB tests and status checks.

Pull Request resolved: #7176

Reviewed By: cheng-chang

Differential Revision: D22799278

Pulled By: ajkr

fbshipit-source-id: 16d8a02eaeecd6df1060249b6a5811292801f2ed
  • Loading branch information
mrambacher authored and facebook-github-bot committed Jul 29, 2020
1 parent c0c33a4 commit d9d1907
Show file tree
Hide file tree
Showing 15 changed files with 111 additions and 93 deletions.
9 changes: 8 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,11 @@ ifdef ASSERT_STATUS_CHECKED
dbformat_test \
defer_test \
dynamic_bloom_test \
env_basic_test \
env_test \
env_logger_test \
event_logger_test \
file_indexer_test \
folly_synchronization_distributed_mutex_test \
hash_table_test \
hash_test \
heap_test \
Expand All @@ -596,13 +598,18 @@ ifdef ASSERT_STATUS_CHECKED
slice_test \
statistics_test \
thread_local_test \
env_timed_test \
timer_queue_test \
timer_test \
util_merge_operators_test \
version_edit_test \
work_queue_test \
write_controller_test \

ifeq ($(USE_FOLLY_DISTRIBUTED_MUTEX),1)
TESTS_PASSING_ASC += folly_synchronization_distributed_mutex_test
endif

# Enable building all unit tests, but use check_some to run only tests
# known to pass ASC
SUBSET := $(TESTS_PASSING_ASC)
Expand Down
2 changes: 1 addition & 1 deletion db/db_impl/db_impl_files.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force,
// set of all files in the directory. We'll exclude files that are still
// alive in the subsequent processings.
std::vector<std::string> files;
env_->GetChildren(path, &files); // Ignore errors
env_->GetChildren(path, &files).PermitUncheckedError(); // Ignore errors
for (const std::string& file : files) {
uint64_t number;
FileType type;
Expand Down
23 changes: 12 additions & 11 deletions env/env_basic_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,13 @@ class EnvBasicTestWithParam : public testing::Test,
test_dir_ = test::PerThreadDBPath(env_, "env_basic_test");
}

void SetUp() override { env_->CreateDirIfMissing(test_dir_); }
void SetUp() override {
env_->CreateDirIfMissing(test_dir_).PermitUncheckedError();
}

void TearDown() override {
std::vector<std::string> files;
env_->GetChildren(test_dir_, &files);
env_->GetChildren(test_dir_, &files).PermitUncheckedError();
for (const auto& file : files) {
// don't know whether it's file or directory, try both. The tests must
// only create files or empty directories, so one must succeed, else the
Expand Down Expand Up @@ -209,13 +211,12 @@ TEST_P(EnvBasicTestWithParam, Basics) {
soptions_)
.ok());
ASSERT_TRUE(!seq_file);
ASSERT_TRUE(!env_->NewRandomAccessFile(test_dir_ + "/non_existent",
&rand_file, soptions_)
.ok());
ASSERT_NOK(env_->NewRandomAccessFile(test_dir_ + "/non_existent", &rand_file,
soptions_));
ASSERT_TRUE(!rand_file);

// Check that deleting works.
ASSERT_TRUE(!env_->DeleteFile(test_dir_ + "/non_existent").ok());
ASSERT_NOK(env_->DeleteFile(test_dir_ + "/non_existent"));
ASSERT_OK(env_->DeleteFile(test_dir_ + "/g"));
ASSERT_EQ(Status::NotFound(), env_->FileExists(test_dir_ + "/g"));
ASSERT_OK(env_->GetChildren(test_dir_, &children));
Expand Down Expand Up @@ -346,14 +347,14 @@ TEST_P(EnvMoreTestWithParam, GetChildren) {
ASSERT_EQ(3U, children.size());
ASSERT_EQ(3U, childAttr.size());
for (auto each : children) {
env_->DeleteDir(test_dir_ + "/" + each);
env_->DeleteDir(test_dir_ + "/" + each).PermitUncheckedError();
} // necessary for default POSIX env

// non-exist directory returns IOError
ASSERT_OK(env_->DeleteDir(test_dir_));
ASSERT_TRUE(!env_->FileExists(test_dir_).ok());
ASSERT_TRUE(!env_->GetChildren(test_dir_, &children).ok());
ASSERT_TRUE(!env_->GetChildrenFileAttributes(test_dir_, &childAttr).ok());
ASSERT_NOK(env_->FileExists(test_dir_));
ASSERT_NOK(env_->GetChildren(test_dir_, &children));
ASSERT_NOK(env_->GetChildrenFileAttributes(test_dir_, &childAttr));

// if dir is a file, returns IOError
ASSERT_OK(env_->CreateDir(test_dir_));
Expand All @@ -362,7 +363,7 @@ TEST_P(EnvMoreTestWithParam, GetChildren) {
env_->NewWritableFile(test_dir_ + "/file", &writable_file, soptions_));
ASSERT_OK(writable_file->Close());
writable_file.reset();
ASSERT_TRUE(!env_->GetChildren(test_dir_ + "/file", &children).ok());
ASSERT_NOK(env_->GetChildren(test_dir_ + "/file", &children));
ASSERT_EQ(0U, children.size());
}

Expand Down
3 changes: 1 addition & 2 deletions env/env_chroot.cc
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,7 @@ class ChrootEnv : public EnvWrapper {
*path = buf;

// Directory may already exist, so ignore return
CreateDir(*path);
return Status::OK();
return CreateDirIfMissing(*path);
}

Status NewLogger(const std::string& fname,
Expand Down
13 changes: 8 additions & 5 deletions env/env_encryption.cc
Original file line number Diff line number Diff line change
Expand Up @@ -472,11 +472,14 @@ class EncryptedEnv : public EnvWrapper {
// Initialize prefix
prefixBuf.Alignment(underlying->GetRequiredBufferAlignment());
prefixBuf.AllocateNewBuffer(prefixLength);
provider_->CreateNewPrefix(fname, prefixBuf.BufferStart(), prefixLength);
prefixBuf.Size(prefixLength);
prefixSlice = Slice(prefixBuf.BufferStart(), prefixBuf.CurrentSize());
// Write prefix
status = underlying->Append(prefixSlice);
status = provider_->CreateNewPrefix(fname, prefixBuf.BufferStart(),
prefixLength);
if (status.ok()) {
prefixBuf.Size(prefixLength);
prefixSlice = Slice(prefixBuf.BufferStart(), prefixBuf.CurrentSize());
// Write prefix
status = underlying->Append(prefixSlice);
}
if (!status.ok()) {
return status;
}
Expand Down
1 change: 0 additions & 1 deletion env/env_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ class PosixEnv : public CompositeEnvWrapper {
// provided by the search path
Status LoadLibrary(const std::string& name, const std::string& path,
std::shared_ptr<DynamicLibrary>* result) override {
Status status;
assert(result != nullptr);
if (name.empty()) {
void* hndl = dlopen(NULL, RTLD_NOW);
Expand Down
Loading

0 comments on commit d9d1907

Please sign in to comment.