From ec3ba734e935eb58263ae21ac11a68265fa08473 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sun, 17 May 2020 14:33:40 +0200 Subject: [PATCH] Don't wrap the main thread when running death tests. Also re-enable the TestAnnotationsOnWrongQueueDebug test and rename the test suite to SequenceCheckerDeathTest so that it gets executed before other tests. Bug: webrtc:11577 Change-Id: I3b8037644e4b9139755ccecb17e42b09327e4996 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175346 Reviewed-by: Mirko Bonadei Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#31290} --- pc/track_media_info_map_unittest.cc | 25 ++++++---- .../sequence_checker_unittest.cc | 8 ++- test/BUILD.gn | 1 + test/test_main_lib.cc | 49 ++++++++++++++----- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/pc/track_media_info_map_unittest.cc b/pc/track_media_info_map_unittest.cc index c487ab8f81..0cb1e0e277 100644 --- a/pc/track_media_info_map_unittest.cc +++ b/pc/track_media_info_map_unittest.cc @@ -83,19 +83,21 @@ rtc::scoped_refptr CreateMockRtpReceiver( class TrackMediaInfoMapTest : public ::testing::Test { public: - TrackMediaInfoMapTest() + TrackMediaInfoMapTest() : TrackMediaInfoMapTest(true) {} + + explicit TrackMediaInfoMapTest(bool use_current_thread) : voice_media_info_(new cricket::VoiceMediaInfo()), video_media_info_(new cricket::VideoMediaInfo()), local_audio_track_(AudioTrack::Create("LocalAudioTrack", nullptr)), remote_audio_track_(AudioTrack::Create("RemoteAudioTrack", nullptr)), - local_video_track_( - VideoTrack::Create("LocalVideoTrack", - FakeVideoTrackSource::Create(false), - rtc::Thread::Current())), - remote_video_track_( - VideoTrack::Create("RemoteVideoTrack", - FakeVideoTrackSource::Create(false), - rtc::Thread::Current())) {} + local_video_track_(VideoTrack::Create( + "LocalVideoTrack", + FakeVideoTrackSource::Create(false), + use_current_thread ? rtc::Thread::Current() : nullptr)), + remote_video_track_(VideoTrack::Create( + "RemoteVideoTrack", + FakeVideoTrackSource::Create(false), + use_current_thread ? rtc::Thread::Current() : nullptr)) {} ~TrackMediaInfoMapTest() { // If we have a map the ownership has been passed to the map, only delete if @@ -417,7 +419,10 @@ TEST_F(TrackMediaInfoMapTest, GetAttachmentIdByTrack) { // base/test/gtest_util.h. #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -class TrackMediaInfoMapDeathTest : public TrackMediaInfoMapTest {}; +class TrackMediaInfoMapDeathTest : public TrackMediaInfoMapTest { + public: + TrackMediaInfoMapDeathTest() : TrackMediaInfoMapTest(false) {} +}; TEST_F(TrackMediaInfoMapDeathTest, MultipleOneSsrcReceiversPerTrack) { AddRtpReceiverWithSsrcs({1}, remote_audio_track_); diff --git a/rtc_base/synchronization/sequence_checker_unittest.cc b/rtc_base/synchronization/sequence_checker_unittest.cc index 3ba7bc7017..a173a825bd 100644 --- a/rtc_base/synchronization/sequence_checker_unittest.cc +++ b/rtc_base/synchronization/sequence_checker_unittest.cc @@ -158,8 +158,12 @@ void TestAnnotationsOnWrongQueue() { } #if RTC_DCHECK_IS_ON -// TODO(bugs.webrtc.org/11577): Fix flakiness. -TEST(SequenceCheckerTest, DISABLED_TestAnnotationsOnWrongQueueDebug) { +// Note: Ending the test suite name with 'DeathTest' is important as it causes +// gtest to order this test before any other non-death-tests, to avoid potential +// global process state pollution such as shared worker threads being started +// (e.g. a side effect of calling InitCocoaMultiThreading() on Mac causes one or +// two additional threads to be created). +TEST(SequenceCheckerDeathTest, TestAnnotationsOnWrongQueueDebug) { ASSERT_DEATH({ TestAnnotationsOnWrongQueue(); }, ""); } #else diff --git a/test/BUILD.gn b/test/BUILD.gn index 34da8894f7..3f91f4a5b2 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -397,6 +397,7 @@ if (rtc_include_tests) { "//third_party/abseil-cpp/absl/flags:flag", "//third_party/abseil-cpp/absl/flags:parse", "//third_party/abseil-cpp/absl/memory", + "//third_party/abseil-cpp/absl/strings:strings", "//third_party/abseil-cpp/absl/types:optional", ] } diff --git a/test/test_main_lib.cc b/test/test_main_lib.cc index 1fd62301e2..f5e02341f3 100644 --- a/test/test_main_lib.cc +++ b/test/test_main_lib.cc @@ -17,6 +17,7 @@ #include "absl/flags/flag.h" #include "absl/flags/parse.h" #include "absl/memory/memory.h" +#include "absl/strings/match.h" #include "absl/types/optional.h" #include "rtc_base/checks.h" #include "rtc_base/event_tracer.h" @@ -109,26 +110,48 @@ class TestMainImpl : public TestMain { TestListener() = default; private: + bool IsDeathTest(const char* test_case_name, const char* test_name) { + // Workaround to avoid wrapping the main thread when we run death tests. + // The approach we take for detecting death tests is essentially the same + // as gtest does internally. Gtest does this: + // + // static const char kDeathTestCaseFilter[] = "*DeathTest:*DeathTest/*"; + // ::testing::internal::UnitTestOptions::MatchesFilter( + // test_case_name, kDeathTestCaseFilter); + // + // Our approach is a little more straight forward. + if (absl::EndsWith(test_case_name, "DeathTest")) + return true; + + return absl::EndsWith(test_name, "DeathTest"); + } + void OnTestStart(const ::testing::TestInfo& test_info) override { - // Ensure that main thread gets wrapped as an rtc::Thread. - // TODO(bugs.webrtc.org/9714): It might be better to avoid wrapping the - // main thread, or leave it to individual tests that need it. But as long - // as we have automatic thread wrapping, we need this to avoid that some - // other random thread (which one depending on which tests are run) gets - // automatically wrapped. - thread_ = rtc::Thread::CreateWithSocketServer(); - thread_->WrapCurrent(); - RTC_DCHECK_EQ(rtc::Thread::Current(), thread_.get()); + if (!IsDeathTest(test_info.test_suite_name(), test_info.name())) { + // Ensure that main thread gets wrapped as an rtc::Thread. + // TODO(bugs.webrtc.org/9714): It might be better to avoid wrapping the + // main thread, or leave it to individual tests that need it. But as + // long as we have automatic thread wrapping, we need this to avoid that + // some other random thread (which one depending on which tests are run) + // gets automatically wrapped. + thread_ = rtc::Thread::CreateWithSocketServer(); + thread_->WrapCurrent(); + RTC_DCHECK_EQ(rtc::Thread::Current(), thread_.get()); + } else { + RTC_LOG(LS_INFO) << "No thread auto wrap for death test."; + } } void OnTestEnd(const ::testing::TestInfo& test_info) override { // Terminate the message loop. Note that if the test failed to clean // up pending messages, this may execute part of the test. Ideally we // should print a warning message here, or even fail the test if it leaks. - thread_->Quit(); // Signal quit. - thread_->Run(); // Flush + process Quit signal. - thread_->UnwrapCurrent(); - thread_ = nullptr; + if (thread_) { + thread_->Quit(); // Signal quit. + thread_->Run(); // Flush + process Quit signal. + thread_->UnwrapCurrent(); + thread_ = nullptr; + } } std::unique_ptr thread_;