Make env*_test work with ASSERT_STATUS_CHECKED (facebook#7176)

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: facebook#7176 Reviewed By: cheng-chang Differential Revision: D22799278 Pulled By: ajkr fbshipit-source-id: 16d8a02eaeecd6df1060249b6a5811292801f2ed
maztheman · Jul 29, 2020 · d9d1907 · d9d1907
1 parent c0c33a4
commit d9d1907
Show file tree

Hide file tree

Showing 15 changed files with 111 additions and 93 deletions.
diff --git a/Makefile b/Makefile
@@ -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 \
@@ -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)

diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc
@@ -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;

diff --git a/env/env_basic_test.cc b/env/env_basic_test.cc
@@ -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
@@ -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));
@@ -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_));
@@ -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());
 }
 

diff --git a/env/env_chroot.cc b/env/env_chroot.cc
@@ -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,

diff --git a/env/env_encryption.cc b/env/env_encryption.cc
@@ -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;
       }

diff --git a/env/env_posix.cc b/env/env_posix.cc
@@ -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);