From fdafab84bc9db9eabdf54b1186c27abfe871801a Mon Sep 17 00:00:00 2001 From: hbos Date: Wed, 14 Sep 2016 06:02:13 -0700 Subject: [PATCH] Fix issues with rtc_stats_unittests tests so that they can run on bots. This target is not run on bots so a couple of issues went under the radar. If we expose the tests and run them on the bots[1] two issues are surfaced which this CL fixes. After this CL lands we can enable this target on the bots without it going red. rtcstats_unittest.cc: Fix const char* string comparison issue by comparing with strcmp instead of equality check. rtcstatscollector_unittest.cc: Fix TSAN issue by constructing ScopedFakeClock before spawning Threads. [1] https://codereview.webrtc.org/2340443002/ BUG=chromium:627816 NOTRY=True Review-Url: https://codereview.webrtc.org/2333343002 Cr-Commit-Position: refs/heads/master@{#14215} --- webrtc/stats/rtcstats_unittest.cc | 57 ++++++++++++++++------ webrtc/stats/rtcstatscollector_unittest.cc | 8 +-- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/webrtc/stats/rtcstats_unittest.cc b/webrtc/stats/rtcstats_unittest.cc index 1d4e016044..f3b4209ea6 100644 --- a/webrtc/stats/rtcstats_unittest.cc +++ b/webrtc/stats/rtcstats_unittest.cc @@ -10,6 +10,8 @@ #include "webrtc/api/rtcstats.h" +#include + #include "webrtc/base/checks.h" #include "webrtc/base/gunit.h" @@ -114,14 +116,30 @@ TEST(RTCStatsTest, RTCStatsAndMembers) { stats.m_double = 123.0; stats.m_static_string = "123"; stats.m_string = std::string("123"); - stats.m_sequence_int32 = std::vector(); - stats.m_sequence_uint32 = std::vector(); + + std::vector sequence_int32; + sequence_int32.push_back(static_cast(1)); + std::vector sequence_uint32; + sequence_uint32.push_back(static_cast(2)); + std::vector sequence_int64; + sequence_int64.push_back(static_cast(3)); + std::vector sequence_uint64; + sequence_uint64.push_back(static_cast(4)); + std::vector sequence_double; + sequence_double.push_back(5.0); + std::vector sequence_static_string; + sequence_static_string.push_back("six"); + std::vector sequence_string; + sequence_string.push_back(std::string("seven")); + + stats.m_sequence_int32 = sequence_int32; + stats.m_sequence_uint32 = sequence_uint32; EXPECT_FALSE(stats.m_sequence_int64.is_defined()); - stats.m_sequence_int64 = std::vector(); - stats.m_sequence_uint64 = std::vector(); - stats.m_sequence_double = std::vector(); - stats.m_sequence_static_string = std::vector(); - stats.m_sequence_string = std::vector(); + stats.m_sequence_int64 = sequence_int64; + stats.m_sequence_uint64 = sequence_uint64; + stats.m_sequence_double = sequence_double; + stats.m_sequence_static_string = sequence_static_string; + stats.m_sequence_string = sequence_string; for (const RTCStatsMemberInterface* member : members) { EXPECT_TRUE(member->is_defined()); } @@ -130,17 +148,24 @@ TEST(RTCStatsTest, RTCStatsAndMembers) { EXPECT_EQ(*stats.m_int64, static_cast(123)); EXPECT_EQ(*stats.m_uint64, static_cast(123)); EXPECT_EQ(*stats.m_double, 123.0); - EXPECT_EQ(*stats.m_static_string, "123"); + EXPECT_EQ(strcmp(*stats.m_static_string, "123"), 0); EXPECT_EQ(*stats.m_string, std::string("123")); - EXPECT_EQ(*stats.m_sequence_int32, std::vector()); - EXPECT_EQ(*stats.m_sequence_uint32, std::vector()); - EXPECT_EQ(*stats.m_sequence_int64, std::vector()); - EXPECT_EQ(*stats.m_sequence_uint64, std::vector()); - EXPECT_EQ(*stats.m_sequence_double, std::vector()); - EXPECT_EQ(*stats.m_sequence_static_string, std::vector()); - EXPECT_EQ(*stats.m_sequence_string, std::vector()); + EXPECT_EQ(*stats.m_sequence_int32, sequence_int32); + EXPECT_EQ(*stats.m_sequence_uint32, sequence_uint32); + EXPECT_EQ(*stats.m_sequence_int64, sequence_int64); + EXPECT_EQ(*stats.m_sequence_uint64, sequence_uint64); + EXPECT_EQ(*stats.m_sequence_double, sequence_double); + EXPECT_EQ(stats.m_sequence_static_string->size(), + sequence_static_string.size()); + for (size_t i = 0; i < sequence_static_string.size(); ++i) { + EXPECT_EQ(strcmp((*stats.m_sequence_static_string)[i], + sequence_static_string[i]), 0); + } + EXPECT_EQ(*stats.m_sequence_string, sequence_string); + int32_t numbers[] = { 4, 8, 15, 16, 23, 42 }; - std::vector numbers_sequence(&numbers[0], &numbers[5]); + std::vector numbers_sequence(&numbers[0], &numbers[6]); + stats.m_sequence_int32->clear(); stats.m_sequence_int32->insert(stats.m_sequence_int32->end(), numbers_sequence.begin(), numbers_sequence.end()); diff --git a/webrtc/stats/rtcstatscollector_unittest.cc b/webrtc/stats/rtcstatscollector_unittest.cc index f3d232ff07..a90c334ae4 100644 --- a/webrtc/stats/rtcstatscollector_unittest.cc +++ b/webrtc/stats/rtcstatscollector_unittest.cc @@ -58,6 +58,7 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver { ReturnRef(data_channels_)); } + rtc::ScopedFakeClock& fake_clock() { return fake_clock_; } MockWebRtcSession& session() { return session_; } MockPeerConnection& pc() { return pc_; } std::vector>& data_channels() { @@ -71,6 +72,7 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver { } private: + rtc::ScopedFakeClock fake_clock_; rtc::Thread* const worker_thread_; rtc::Thread* const network_thread_; std::unique_ptr channel_manager_; @@ -277,7 +279,6 @@ TEST_F(RTCStatsCollectorTest, MultipleCallbacks) { } TEST_F(RTCStatsCollectorTest, CachedStatsReports) { - rtc::ScopedFakeClock fake_clock; // Caching should ensure |a| and |b| are the same report. rtc::scoped_refptr a = GetStatsReport(); rtc::scoped_refptr b = GetStatsReport(); @@ -287,21 +288,20 @@ TEST_F(RTCStatsCollectorTest, CachedStatsReports) { rtc::scoped_refptr c = GetStatsReport(); EXPECT_NE(b.get(), c.get()); // Invalidate cache by advancing time. - fake_clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(51)); + test_->fake_clock().AdvanceTime(rtc::TimeDelta::FromMilliseconds(51)); rtc::scoped_refptr d = GetStatsReport(); EXPECT_TRUE(d); EXPECT_NE(c.get(), d.get()); } TEST_F(RTCStatsCollectorTest, MultipleCallbacksWithInvalidatedCacheInBetween) { - rtc::ScopedFakeClock fake_clock; rtc::scoped_refptr a; rtc::scoped_refptr b; rtc::scoped_refptr c; collector_->GetStatsReport(StatsCallback::Create(&a)); collector_->GetStatsReport(StatsCallback::Create(&b)); // Cache is invalidated after 50 ms. - fake_clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(51)); + test_->fake_clock().AdvanceTime(rtc::TimeDelta::FromMilliseconds(51)); collector_->GetStatsReport(StatsCallback::Create(&c)); EXPECT_TRUE_WAIT(a, kGetStatsReportTimeoutMs); EXPECT_TRUE_WAIT(b, kGetStatsReportTimeoutMs);