From 89ea7f26438001a796f3338222e5321c5571042f Mon Sep 17 00:00:00 2001 From: Reilly Grant Date: Tue, 28 Mar 2023 14:37:48 -0700 Subject: [PATCH 1/2] Fix tests when run against ChromiumEnv There are a couple differences between ChromiumEnv and PosixEnv/WindowsEnv which cause test failures that are fixed (or at least patched over) in this change: * NewSequentialFile() and NewRandomAccessFile() return Status::IOError rather than Status::NotFound when a file is not found, due to https://crbug.com/760362. This means a few tests need to expect a different error result. * GetChildren() never returns the '.' or '..' entries. * As allowed by the documentation for Env::Schedule(), ChromiumEnv may execute functions on multiple threads and guarantees no sequencing. EnvTest.RunMany assumed that functions ran in order. The test has been updated. --- db/db_test.cc | 5 +++++ db/recovery_test.cc | 5 +++++ util/env_test.cc | 37 ++++++++++++++++++++++++++----------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 472258b..a4d8d58 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -1722,8 +1722,13 @@ TEST_F(DBTest, DestroyEmptyDir) { ASSERT_TRUE(env.FileExists(dbname)); std::vector children; ASSERT_LEVELDB_OK(env.GetChildren(dbname, &children)); +#if defined(LEVELDB_PLATFORM_CHROMIUM) + // Chromium's file system abstraction always filters out '.' and '..'. + ASSERT_EQ(0, children.size()); +#else // The stock Env's do not filter out '.' and '..' special files. ASSERT_EQ(2, children.size()); +#endif // defined(LEVELDB_PLATFORM_CHROMIUM) ASSERT_LEVELDB_OK(DestroyDB(dbname, opts)); ASSERT_TRUE(!env.FileExists(dbname)); diff --git a/db/recovery_test.cc b/db/recovery_test.cc index 1d9f621..8dc039a 100644 --- a/db/recovery_test.cc +++ b/db/recovery_test.cc @@ -328,7 +328,12 @@ TEST_F(RecoveryTest, ManifestMissing) { RemoveManifestFile(); Status status = OpenWithStatus(); +#if defined(LEVELDB_PLATFORM_CHROMIUM) + // TODO(crbug.com/760362): See comment in MakeIOError() from env_chromium.cc. + ASSERT_TRUE(status.IsIOError()); +#else ASSERT_TRUE(status.IsCorruption()); +#endif // defined(LEVELDB_PLATFORM_CHROMIUM) } } // namespace leveldb diff --git a/util/env_test.cc b/util/env_test.cc index 47174f5..e5c30fe 100644 --- a/util/env_test.cc +++ b/util/env_test.cc @@ -96,40 +96,45 @@ TEST_F(EnvTest, RunMany) { struct RunState { port::Mutex mu; port::CondVar cvar{&mu}; - int last_id = 0; + int run_count = 0; }; struct Callback { - RunState* state_; // Pointer to shared state. - const int id_; // Order# for the execution of this callback. + RunState* const state_; // Pointer to shared state. + bool run = false; - Callback(RunState* s, int id) : state_(s), id_(id) {} + Callback(RunState* s) : state_(s) {} static void Run(void* arg) { Callback* callback = reinterpret_cast(arg); RunState* state = callback->state_; MutexLock l(&state->mu); - ASSERT_EQ(state->last_id, callback->id_ - 1); - state->last_id = callback->id_; + state->run_count++; + callback->run = true; state->cvar.Signal(); } }; RunState state; - Callback callback1(&state, 1); - Callback callback2(&state, 2); - Callback callback3(&state, 3); - Callback callback4(&state, 4); + Callback callback1(&state); + Callback callback2(&state); + Callback callback3(&state); + Callback callback4(&state); env_->Schedule(&Callback::Run, &callback1); env_->Schedule(&Callback::Run, &callback2); env_->Schedule(&Callback::Run, &callback3); env_->Schedule(&Callback::Run, &callback4); MutexLock l(&state.mu); - while (state.last_id != 4) { + while (state.run_count != 4) { state.cvar.Wait(); } + + ASSERT_TRUE(callback1.run); + ASSERT_TRUE(callback2.run); + ASSERT_TRUE(callback3.run); + ASSERT_TRUE(callback4.run); } struct State { @@ -175,11 +180,21 @@ TEST_F(EnvTest, TestOpenNonExistentFile) { RandomAccessFile* random_access_file; Status status = env_->NewRandomAccessFile(non_existent_file, &random_access_file); +#if defined(LEVELDB_PLATFORM_CHROMIUM) + // TODO(crbug.com/760362): See comment in MakeIOError() from env_chromium.cc. + ASSERT_TRUE(status.IsIOError()); +#else ASSERT_TRUE(status.IsNotFound()); +#endif // defined(LEVELDB_PLATFORM_CHROMIUM) SequentialFile* sequential_file; status = env_->NewSequentialFile(non_existent_file, &sequential_file); +#if defined(LEVELDB_PLATFORM_CHROMIUM) + // TODO(crbug.com/760362): See comment in MakeIOError() from env_chromium.cc. + ASSERT_TRUE(status.IsIOError()); +#else ASSERT_TRUE(status.IsNotFound()); +#endif // defined(LEVELDB_PLATFORM_CHROMIUM) } TEST_F(EnvTest, ReopenWritableFile) { From 13ebad24dc1a5b338b619e8f1229105fbba3992f Mon Sep 17 00:00:00 2001 From: Reilly Grant Date: Tue, 28 Mar 2023 16:20:01 -0700 Subject: [PATCH 2/2] Address comments. --- db/db_test.cc | 3 ++- util/env_test.cc | 27 +++++++++++---------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index a4d8d58..a4a84cd 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -1723,7 +1723,8 @@ TEST_F(DBTest, DestroyEmptyDir) { std::vector children; ASSERT_LEVELDB_OK(env.GetChildren(dbname, &children)); #if defined(LEVELDB_PLATFORM_CHROMIUM) - // Chromium's file system abstraction always filters out '.' and '..'. + // TODO(https://crbug.com/1428746): Chromium's file system abstraction always + // filters out '.' and '..'. ASSERT_EQ(0, children.size()); #else // The stock Env's do not filter out '.' and '..' special files. diff --git a/util/env_test.cc b/util/env_test.cc index e5c30fe..a65bf8e 100644 --- a/util/env_test.cc +++ b/util/env_test.cc @@ -96,45 +96,40 @@ TEST_F(EnvTest, RunMany) { struct RunState { port::Mutex mu; port::CondVar cvar{&mu}; - int run_count = 0; + int last_id = 0; }; struct Callback { - RunState* const state_; // Pointer to shared state. - bool run = false; + RunState* state_; // Pointer to shared state. + const int id_; // Order# for the execution of this callback. - Callback(RunState* s) : state_(s) {} + Callback(RunState* s, int id) : state_(s), id_(id) {} static void Run(void* arg) { Callback* callback = reinterpret_cast(arg); RunState* state = callback->state_; MutexLock l(&state->mu); - state->run_count++; - callback->run = true; + ASSERT_EQ(state->last_id, callback->id_ - 1); + state->last_id = callback->id_; state->cvar.Signal(); } }; RunState state; - Callback callback1(&state); - Callback callback2(&state); - Callback callback3(&state); - Callback callback4(&state); + Callback callback1(&state, 1); + Callback callback2(&state, 2); + Callback callback3(&state, 3); + Callback callback4(&state, 4); env_->Schedule(&Callback::Run, &callback1); env_->Schedule(&Callback::Run, &callback2); env_->Schedule(&Callback::Run, &callback3); env_->Schedule(&Callback::Run, &callback4); MutexLock l(&state.mu); - while (state.run_count != 4) { + while (state.last_id != 4) { state.cvar.Wait(); } - - ASSERT_TRUE(callback1.run); - ASSERT_TRUE(callback2.run); - ASSERT_TRUE(callback3.run); - ASSERT_TRUE(callback4.run); } struct State {