From 2214b91c1f9693e6382634fdc05d56eaddb4668a Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 25 Mar 2019 15:13:59 +0100 Subject: [PATCH] Cleanup SequenceTaskChecker unittests Replaced multiple helpers for running code on a different thread with single helper. Move logic under test and expectation from helpers to tests Remove unique_ptr indirection where not used. Replace rtc::TaskQueue with TaskQueueForTest to keep use convenient constructor and to use convenient synchronous calls Bug: webrtc:10284 Change-Id: I3f1ec2adc4dc16b44d35e769ef8b456407ee66c2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/128903 Reviewed-by: Karl Wiberg Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#27280} --- rtc_base/BUILD.gn | 4 +- rtc_base/sequenced_task_checker_unittest.cc | 257 ++++++-------------- 2 files changed, 71 insertions(+), 190 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 06a705c48f..d362c98db7 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1299,9 +1299,11 @@ if (rtc_include_tests) { ":checks", ":rtc_base_approved", ":rtc_base_tests_main", - ":rtc_task_queue", ":sequenced_task_checker", + ":task_queue_for_test", + "../api:function_view", "../test:test_support", + "//third_party/abseil-cpp/absl/memory", ] } diff --git a/rtc_base/sequenced_task_checker_unittest.cc b/rtc_base/sequenced_task_checker_unittest.cc index b3df2fab8b..74cba8a4d5 100644 --- a/rtc_base/sequenced_task_checker_unittest.cc +++ b/rtc_base/sequenced_task_checker_unittest.cc @@ -13,18 +13,19 @@ #include #include -#include "rtc_base/checks.h" -#include "rtc_base/constructor_magic.h" +#include "absl/memory/memory.h" +#include "api/function_view.h" #include "rtc_base/event.h" #include "rtc_base/platform_thread.h" -#include "rtc_base/task_queue.h" +#include "rtc_base/task_queue_for_test.h" #include "rtc_base/thread_checker.h" #include "test/gtest.h" namespace rtc { - namespace { +using ::webrtc::TaskQueueForTest; + // This class is dead code, but its purpose is to make sure that // SequencedTaskChecker is compatible with the RTC_GUARDED_BY and RTC_RUN_ON // attributes that are checked at compile-time. @@ -34,219 +35,103 @@ class CompileTimeTestForGuardedBy { void CallMeFromSequence() { RTC_DCHECK_RUN_ON(&sequence_checker_) << "Should be called on sequence"; + guarded_ = 41; } private: int guarded_ RTC_GUARDED_BY(sequence_checker_); - rtc::SequencedTaskChecker sequence_checker_; + SequencedTaskChecker sequence_checker_; }; -// Calls SequencedTaskChecker::CalledSequentially on another thread. -class CallCalledSequentiallyOnThread { - public: - CallCalledSequentiallyOnThread(bool expect_true, - SequencedTaskChecker* sequenced_task_checker) - : expect_true_(expect_true), - thread_(&Run, this, "call_do_stuff_on_thread"), - sequenced_task_checker_(sequenced_task_checker) { - thread_.Start(); - } - ~CallCalledSequentiallyOnThread() { - EXPECT_TRUE(thread_has_run_event_.Wait(1000)); - thread_.Stop(); - } +void RunOnDifferentThread(FunctionView run) { + struct Object { + static void Run(void* obj) { + auto* me = static_cast(obj); + me->run(); + me->thread_has_run_event.Set(); + } - private: - static void Run(void* obj) { - CallCalledSequentiallyOnThread* call_stuff_on_thread = - static_cast(obj); - EXPECT_EQ( - call_stuff_on_thread->expect_true_, - call_stuff_on_thread->sequenced_task_checker_->CalledSequentially()); - call_stuff_on_thread->thread_has_run_event_.Set(); - } + FunctionView run; + Event thread_has_run_event; + } object{run}; - const bool expect_true_; - Event thread_has_run_event_; - PlatformThread thread_; - SequencedTaskChecker* const sequenced_task_checker_; - - RTC_DISALLOW_COPY_AND_ASSIGN(CallCalledSequentiallyOnThread); -}; - -// Deletes SequencedTaskChecker on a different thread. -class DeleteSequencedCheckerOnThread { - public: - explicit DeleteSequencedCheckerOnThread( - std::unique_ptr sequenced_task_checker) - : thread_(&Run, this, "delete_sequenced_task_checker_on_thread"), - sequenced_task_checker_(std::move(sequenced_task_checker)) { - thread_.Start(); - } - - ~DeleteSequencedCheckerOnThread() { - EXPECT_TRUE(thread_has_run_event_.Wait(1000)); - thread_.Stop(); - } - - private: - static bool Run(void* obj) { - DeleteSequencedCheckerOnThread* instance = - static_cast(obj); - instance->sequenced_task_checker_.reset(); - instance->thread_has_run_event_.Set(); - return false; - } - - private: - PlatformThread thread_; - Event thread_has_run_event_; - std::unique_ptr sequenced_task_checker_; - - RTC_DISALLOW_COPY_AND_ASSIGN(DeleteSequencedCheckerOnThread); -}; - -void RunMethodOnDifferentThread(bool expect_true) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - CallCalledSequentiallyOnThread call_on_thread(expect_true, - sequenced_task_checker.get()); + PlatformThread thread(&Object::Run, &object, "thread"); + thread.Start(); + EXPECT_TRUE(object.thread_has_run_event.Wait(1000)); + thread.Stop(); } -void RunMethodOnDifferentTaskQueue(bool expect_true) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - static const char kQueueName[] = "MethodNotAllowedOnDifferentTq"; - TaskQueue queue(kQueueName); - Event done_event; - queue.PostTask([&sequenced_task_checker, &done_event, expect_true] { - if (expect_true) - EXPECT_TRUE(sequenced_task_checker->CalledSequentially()); - else - EXPECT_FALSE(sequenced_task_checker->CalledSequentially()); - done_event.Set(); - }); - EXPECT_TRUE(done_event.Wait(1000)); -} - -void DetachThenCallFromDifferentTaskQueue(bool expect_true) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - sequenced_task_checker->Detach(); - - Event done_event; - TaskQueue queue1("DetachThenCallFromDifferentTaskQueueImpl1"); - queue1.PostTask([&sequenced_task_checker, &done_event] { - EXPECT_TRUE(sequenced_task_checker->CalledSequentially()); - done_event.Set(); - }); - EXPECT_TRUE(done_event.Wait(1000)); - - // CalledSequentially should return false in debug builds after moving to - // another task queue. - TaskQueue queue2("DetachThenCallFromDifferentTaskQueueImpl2"); - queue2.PostTask([&sequenced_task_checker, &done_event, expect_true] { - if (expect_true) - EXPECT_TRUE(sequenced_task_checker->CalledSequentially()); - else - EXPECT_FALSE(sequenced_task_checker->CalledSequentially()); - done_event.Set(); - }); - EXPECT_TRUE(done_event.Wait(1000)); -} } // namespace TEST(SequencedTaskCheckerTest, CallsAllowedOnSameThread) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - EXPECT_TRUE(sequenced_task_checker->CalledSequentially()); - - // Verify that the destructor doesn't assert. - sequenced_task_checker.reset(); + SequencedTaskChecker sequenced_task_checker; + EXPECT_TRUE(sequenced_task_checker.CalledSequentially()); } TEST(SequencedTaskCheckerTest, DestructorAllowedOnDifferentThread) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - // Verify that the destructor doesn't assert when called on a different - // thread. - DeleteSequencedCheckerOnThread delete_on_thread( - std::move(sequenced_task_checker)); + auto sequenced_task_checker = absl::make_unique(); + RunOnDifferentThread([&] { + // Verify that the destructor doesn't assert when called on a different + // thread. + sequenced_task_checker.reset(); + }); } TEST(SequencedTaskCheckerTest, DetachFromThread) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - sequenced_task_checker->Detach(); - CallCalledSequentiallyOnThread call_on_thread(true, - sequenced_task_checker.get()); + SequencedTaskChecker sequenced_task_checker; + sequenced_task_checker.Detach(); + RunOnDifferentThread( + [&] { EXPECT_TRUE(sequenced_task_checker.CalledSequentially()); }); } TEST(SequencedTaskCheckerTest, DetachFromThreadAndUseOnTaskQueue) { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - sequenced_task_checker->Detach(); - static const char kQueueName[] = "DetachFromThreadAndUseOnTaskQueue"; - TaskQueue queue(kQueueName); - Event done_event; - queue.PostTask([&sequenced_task_checker, &done_event] { - EXPECT_TRUE(sequenced_task_checker->CalledSequentially()); - done_event.Set(); - }); - EXPECT_TRUE(done_event.Wait(1000)); + SequencedTaskChecker sequenced_task_checker; + sequenced_task_checker.Detach(); + TaskQueueForTest queue; + queue.SendTask( + [&] { EXPECT_TRUE(sequenced_task_checker.CalledSequentially()); }); } TEST(SequencedTaskCheckerTest, DetachFromTaskQueueAndUseOnThread) { - TaskQueue queue("DetachFromTaskQueueAndUseOnThread"); - Event done_event; - queue.PostTask([&done_event] { - std::unique_ptr sequenced_task_checker( - new SequencedTaskChecker()); - - sequenced_task_checker->Detach(); - CallCalledSequentiallyOnThread call_on_thread(true, - sequenced_task_checker.get()); - done_event.Set(); + TaskQueueForTest queue; + queue.SendTask([] { + SequencedTaskChecker sequenced_task_checker; + sequenced_task_checker.Detach(); + RunOnDifferentThread( + [&] { EXPECT_TRUE(sequenced_task_checker.CalledSequentially()); }); }); - EXPECT_TRUE(done_event.Wait(1000)); } -#if RTC_DCHECK_IS_ON TEST(SequencedTaskCheckerTest, MethodNotAllowedOnDifferentThreadInDebug) { - RunMethodOnDifferentThread(false); + SequencedTaskChecker sequenced_task_checker; + RunOnDifferentThread([&] { + EXPECT_EQ(sequenced_task_checker.CalledSequentially(), !RTC_DCHECK_IS_ON); + }); } -#else -TEST(SequencedTaskCheckerTest, MethodAllowedOnDifferentThreadInRelease) { - RunMethodOnDifferentThread(true); -} -#endif -#if RTC_DCHECK_IS_ON TEST(SequencedTaskCheckerTest, MethodNotAllowedOnDifferentTaskQueueInDebug) { - RunMethodOnDifferentTaskQueue(false); + SequencedTaskChecker sequenced_task_checker; + TaskQueueForTest queue; + queue.SendTask([&] { + EXPECT_EQ(sequenced_task_checker.CalledSequentially(), !RTC_DCHECK_IS_ON); + }); } -#else -TEST(SequencedTaskCheckerTest, MethodAllowedOnDifferentTaskQueueInRelease) { - RunMethodOnDifferentTaskQueue(true); -} -#endif -#if RTC_DCHECK_IS_ON TEST(SequencedTaskCheckerTest, DetachFromTaskQueueInDebug) { - DetachThenCallFromDifferentTaskQueue(false); + SequencedTaskChecker sequenced_task_checker; + sequenced_task_checker.Detach(); + + TaskQueueForTest queue1; + queue1.SendTask( + [&] { EXPECT_TRUE(sequenced_task_checker.CalledSequentially()); }); + + // CalledSequentially should return false in debug builds after moving to + // another task queue. + TaskQueueForTest queue2; + queue2.SendTask([&] { + EXPECT_EQ(sequenced_task_checker.CalledSequentially(), !RTC_DCHECK_IS_ON); + }); } -#else -TEST(SequencedTaskCheckerTest, DetachFromTaskQueueInRelease) { - DetachThenCallFromDifferentTaskQueue(true); -} -#endif class TestAnnotations { public: @@ -271,14 +156,8 @@ TEST(SequencedTaskCheckerTest, TestAnnotations) { void TestAnnotationsOnWrongQueue() { TestAnnotations annotations; - static const char kQueueName[] = "TestAnnotationsOnWrongQueueDebug"; - TaskQueue queue(kQueueName); - Event done_event; - queue.PostTask([&annotations, &done_event] { - annotations.ModifyTestVar(); - done_event.Set(); - }); - EXPECT_TRUE(done_event.Wait(1000)); + TaskQueueForTest queue; + queue.SendTask([&] { annotations.ModifyTestVar(); }); } #if RTC_DCHECK_IS_ON