From f7b30e046e3d2a16b50b8139d6eae0d261d8e766 Mon Sep 17 00:00:00 2001 From: Tommi Date: Wed, 6 Jul 2022 12:26:48 +0200 Subject: [PATCH] A few cleanup things for the port classes to clarify test code. Remove FlushRequestsForTest Rename test constant Remove HasPendingRequestForTest Bug: webrtc:13892 Change-Id: I78e13d229742c40743465b5fb57480c24d7417c1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258722 Auto-Submit: Tomas Gunnarsson Commit-Queue: Tomas Gunnarsson Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#37466} --- p2p/base/stun_port.h | 4 ---- p2p/base/stun_port_unittest.cc | 14 ++++++++------ p2p/base/stun_request.cc | 5 +++-- p2p/base/stun_request.h | 8 ++++---- p2p/base/turn_port.h | 4 ---- p2p/base/turn_port_unittest.cc | 8 ++++---- 6 files changed, 19 insertions(+), 24 deletions(-) diff --git a/p2p/base/stun_port.h b/p2p/base/stun_port.h index 83a90fa955..7a3239ee95 100644 --- a/p2p/base/stun_port.h +++ b/p2p/base/stun_port.h @@ -114,10 +114,6 @@ class UDPPort : public Port { void set_stun_keepalive_lifetime(int lifetime) { stun_keepalive_lifetime_ = lifetime; } - // Returns true if there is a pending request with type `msg_type`. - bool HasPendingRequestForTest(int msg_type) { - return request_manager_.HasRequestForTest(msg_type); - } StunRequestManager& request_manager() { return request_manager_; } diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index fa51ed6666..d9c6500192 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc @@ -64,6 +64,10 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> { bool done() const { return done_; } bool error() const { return error_; } + bool HasPendingRequest(int msg_type) { + return stun_port_->request_manager().HasRequestForTest(msg_type); + } + void SetNetworkType(rtc::AdapterType adapter_type) { network_.set_type(adapter_type); } @@ -392,9 +396,8 @@ TEST_F(StunPortTest, TestStunBindingRequestShortLifetime) { CreateStunPort(kStunAddr1); PrepareAddress(); EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); - EXPECT_TRUE_SIMULATED_WAIT( - !port()->HasPendingRequestForTest(cricket::STUN_BINDING_REQUEST), 2000, - fake_clock); + EXPECT_TRUE_SIMULATED_WAIT(!HasPendingRequest(cricket::STUN_BINDING_REQUEST), + 2000, fake_clock); } // Test that by default, the STUN binding requests will last for a long time. @@ -403,9 +406,8 @@ TEST_F(StunPortTest, TestStunBindingRequestLongLifetime) { CreateStunPort(kStunAddr1); PrepareAddress(); EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); - EXPECT_TRUE_SIMULATED_WAIT( - port()->HasPendingRequestForTest(cricket::STUN_BINDING_REQUEST), 1000, - fake_clock); + EXPECT_TRUE_SIMULATED_WAIT(HasPendingRequest(cricket::STUN_BINDING_REQUEST), + 1000, fake_clock); } class MockAsyncPacketSocket : public rtc::AsyncPacketSocket { diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index b4bce73023..3fe02aff22 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -65,7 +65,7 @@ void StunRequestManager::SendDelayed(StunRequest* request, int delay) { void StunRequestManager::FlushForTest(int msg_type) { RTC_DCHECK_RUN_ON(thread_); for (const auto& [unused, request] : requests_) { - if (msg_type == kAllRequests || msg_type == request->type()) { + if (msg_type == kAllRequestsForTest || msg_type == request->type()) { // Calling `Send` implies starting the send operation which may be posted // on a timer and be repeated on a timer until timeout. To make sure that // a call to `Send` doesn't conflict with a previously started `Send` @@ -80,8 +80,9 @@ void StunRequestManager::FlushForTest(int msg_type) { bool StunRequestManager::HasRequestForTest(int msg_type) { RTC_DCHECK_RUN_ON(thread_); + RTC_DCHECK_NE(msg_type, kAllRequestsForTest); for (const auto& [unused, request] : requests_) { - if (msg_type == kAllRequests || msg_type == request->type()) { + if (msg_type == request->type()) { return true; } } diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h index cc1e1bf332..6e83be3830 100644 --- a/p2p/base/stun_request.h +++ b/p2p/base/stun_request.h @@ -28,7 +28,7 @@ namespace cricket { class StunRequest; -const int kAllRequests = 0; +const int kAllRequestsForTest = 0; // Total max timeouts: 39.75 seconds // For years, this was 9.5 seconds, but for networks that experience moments of @@ -48,9 +48,9 @@ class StunRequestManager { void Send(StunRequest* request); void SendDelayed(StunRequest* request, int delay); - // If `msg_type` is kAllRequests, sends all pending requests right away. - // Otherwise, sends those that have a matching type right away. - // Only for testing. + // If `msg_type` is kAllRequestsForTest, sends all pending requests right + // away. Otherwise, sends those that have a matching type right away. Only for + // testing. // TODO(tommi): Remove this method and update tests that use it to simulate // production code. void FlushForTest(int msg_type); diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index 8a77b9f15b..2e61d361da 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -191,10 +191,6 @@ class TurnPort : public Port { sigslot::signal3 SignalCreatePermissionResult; - void FlushRequestsForTest(int msg_type) { - request_manager_.FlushForTest(msg_type); - } - bool HasRequests() { return !request_manager_.empty(); } void set_credentials(const RelayCredentials& credentials) { credentials_ = credentials; diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index ff529cbb93..ef9b1d4cd7 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -1244,10 +1244,10 @@ TEST_F(TurnPortTest, TestRefreshRequestGetsErrorResponse) { // This sends out the first RefreshRequest with correct credentials. // When this succeeds, it will schedule a new RefreshRequest with the bad // credential. - turn_port_->FlushRequestsForTest(TURN_REFRESH_REQUEST); + turn_port_->request_manager().FlushForTest(TURN_REFRESH_REQUEST); EXPECT_TRUE_SIMULATED_WAIT(turn_refresh_success_, kSimulatedRtt, fake_clock_); // Flush it again, it will receive a bad response. - turn_port_->FlushRequestsForTest(TURN_REFRESH_REQUEST); + turn_port_->request_manager().FlushForTest(TURN_REFRESH_REQUEST); EXPECT_TRUE_SIMULATED_WAIT(!turn_refresh_success_, kSimulatedRtt, fake_clock_); EXPECT_FALSE(turn_port_->connected()); @@ -1461,11 +1461,11 @@ TEST_F(TurnPortTest, TestRefreshCreatePermissionRequest) { // another request with bad_ufrag and bad_pwd. RelayCredentials bad_credentials("bad_user", "bad_pwd"); turn_port_->set_credentials(bad_credentials); - turn_port_->FlushRequestsForTest(kAllRequests); + turn_port_->request_manager().FlushForTest(kAllRequestsForTest); EXPECT_TRUE_SIMULATED_WAIT(turn_create_permission_success_, kSimulatedRtt, fake_clock_); // Flush the requests again; the create-permission-request will fail. - turn_port_->FlushRequestsForTest(kAllRequests); + turn_port_->request_manager().FlushForTest(kAllRequestsForTest); EXPECT_TRUE_SIMULATED_WAIT(!turn_create_permission_success_, kSimulatedRtt, fake_clock_); EXPECT_TRUE(CheckConnectionFailedAndPruned(conn));