From 083fc79d26bc1792c060fe8d35a1d3c42f4745f1 Mon Sep 17 00:00:00 2001 From: Yuri Wiitala Date: Wed, 26 Jun 2019 16:03:33 -0700 Subject: Eliminate openscreen_unittests log spam, fix some gmock warnings. Use ::testing::NiceMock and/or correct unit tests that were missing EXPECT_CALL()'s that were actually relevant to the test. Also, raise the default log level to kWarning (was kInfo); but all command-line tools and demo apps explicitly set this back down to kInfo. Background: Many of the tests were spamming the logs with "uninteresting mock" warnings from GMock. Also, there was a ton of spam due to INFO logging meant for the command-line tools. This made it difficult for humans to find the logging output associated with test failures. Change-Id: I2560a7324130727c25e0593b86d08b4753e0570e Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1674509 Reviewed-by: Yuri Wiitala Reviewed-by: Max Yakimakha Reviewed-by: Ryan Keane Reviewed-by: mark a. foltz Commit-Queue: Yuri Wiitala --- .../presentation_connection_unittest.cc | 11 +++++--- osp/impl/presentation/presentation_controller.cc | 4 +-- .../presentation_controller_unittest.cc | 9 +++++-- .../presentation/presentation_receiver_unittest.cc | 4 ++- .../testing/mock_connection_delegate.h | 2 +- .../url_availability_requester_unittest.cc | 30 +++++++++++++++------- 6 files changed, 42 insertions(+), 18 deletions(-) (limited to 'osp/impl/presentation') diff --git a/osp/impl/presentation/presentation_connection_unittest.cc b/osp/impl/presentation/presentation_connection_unittest.cc index e52fed48..0c5096cf 100644 --- a/osp/impl/presentation/presentation_connection_unittest.cc +++ b/osp/impl/presentation/presentation_connection_unittest.cc @@ -22,8 +22,11 @@ namespace presentation { using ::testing::_; using ::testing::Invoke; +using ::testing::NiceMock; -class MockParentDelegate final : public Connection::ParentDelegate { +namespace { + +class MockParentDelegate : public Connection::ParentDelegate { public: MockParentDelegate() = default; ~MockParentDelegate() override = default; @@ -49,6 +52,8 @@ class MockConnectRequest final MOCK_METHOD1(OnConnectionFailed, void(uint64_t request_id)); }; +} // namespace + class ConnectionTest : public ::testing::Test { protected: void SetUp() override { @@ -76,8 +81,8 @@ class ConnectionTest : public ::testing::Test { quic_bridge_.controller_demuxer.get()}; ConnectionManager receiver_connection_manager_{ quic_bridge_.receiver_demuxer.get()}; - MockParentDelegate mock_controller_; - MockParentDelegate mock_receiver_; + NiceMock mock_controller_; + NiceMock mock_receiver_; }; TEST_F(ConnectionTest, ConnectAndSend) { diff --git a/osp/impl/presentation/presentation_controller.cc b/osp/impl/presentation/presentation_controller.cc index de10290b..fa62a3df 100644 --- a/osp/impl/presentation/presentation_controller.cc +++ b/osp/impl/presentation/presentation_controller.cc @@ -679,8 +679,8 @@ ErrorOr Controller::TerminationListener::OnStreamMessage( // static std::string Controller::MakePresentationId(const std::string& url, const std::string& service_id) { - OSP_UNIMPLEMENTED(); - // TODO(btolsch): This is just a placeholder for the demo. + // TODO(btolsch): This is just a placeholder for the demo. It should + // eventually become a GUID/unguessable token routine. std::string safe_id = service_id; for (auto& c : safe_id) if (c < ' ' || c > '~') diff --git a/osp/impl/presentation/presentation_controller_unittest.cc b/osp/impl/presentation/presentation_controller_unittest.cc index e1758c85..5b09d10c 100644 --- a/osp/impl/presentation/presentation_controller_unittest.cc +++ b/osp/impl/presentation/presentation_controller_unittest.cc @@ -23,6 +23,9 @@ namespace presentation { using std::chrono::seconds; using ::testing::_; using ::testing::Invoke; +using ::testing::NiceMock; + +namespace { const char kTestUrl[] = "https://example.foo"; @@ -68,6 +71,8 @@ class MockRequestDelegate final : public RequestDelegate { MOCK_METHOD1(OnError, void(const Error& error)); }; +} // namespace + class ControllerTest : public ::testing::Test { protected: void SetUp() override { @@ -381,7 +386,7 @@ TEST_F(ControllerTest, ReceiverWatchCancel) { TEST_F(ControllerTest, StartPresentation) { MockMessageCallback mock_callback; - MockConnectionDelegate mock_connection_delegate; + NiceMock mock_connection_delegate; std::unique_ptr connection; StartPresentation(&mock_callback, &mock_connection_delegate, &connection); } @@ -461,7 +466,7 @@ TEST_F(ControllerTest, CloseConnection) { TEST_F(ControllerTest, Reconnect) { MockMessageCallback mock_callback; - MockConnectionDelegate mock_connection_delegate; + NiceMock mock_connection_delegate; std::unique_ptr connection; StartPresentation(&mock_callback, &mock_connection_delegate, &connection); diff --git a/osp/impl/presentation/presentation_receiver_unittest.cc b/osp/impl/presentation/presentation_receiver_unittest.cc index 9a6e0dbd..51e01aa9 100644 --- a/osp/impl/presentation/presentation_receiver_unittest.cc +++ b/osp/impl/presentation/presentation_receiver_unittest.cc @@ -20,10 +20,12 @@ namespace openscreen { namespace presentation { + namespace { using ::testing::_; using ::testing::Invoke; +using ::testing::NiceMock; class MockConnectRequest final : public ProtocolConnectionClient::ConnectionRequestCallback { @@ -170,7 +172,7 @@ TEST_F(PresentationReceiverTest, StartPresentation) { EXPECT_EQ(presentation_id, info.id); EXPECT_EQ(url1_, info.url); - MockConnectionDelegate null_connection_delegate; + NiceMock null_connection_delegate; Connection connection(Connection::PresentationInfo{presentation_id, url1_}, &null_connection_delegate, Receiver::Get()); Receiver::Get()->OnPresentationStarted(presentation_id, &connection, diff --git a/osp/impl/presentation/testing/mock_connection_delegate.h b/osp/impl/presentation/testing/mock_connection_delegate.h index 85c3c113..08960686 100644 --- a/osp/impl/presentation/testing/mock_connection_delegate.h +++ b/osp/impl/presentation/testing/mock_connection_delegate.h @@ -11,7 +11,7 @@ namespace openscreen { namespace presentation { -class MockConnectionDelegate final : public Connection::Delegate { +class MockConnectionDelegate : public Connection::Delegate { public: ~MockConnectionDelegate() override = default; diff --git a/osp/impl/presentation/url_availability_requester_unittest.cc b/osp/impl/presentation/url_availability_requester_unittest.cc index 5b8c2f8f..9732fc38 100644 --- a/osp/impl/presentation/url_availability_requester_unittest.cc +++ b/osp/impl/presentation/url_availability_requester_unittest.cc @@ -24,9 +24,12 @@ namespace presentation { using ::testing::_; using ::testing::Invoke; +using ::testing::Mock; using ::testing::Test; -class MockReceiverObserver final : public ReceiverObserver { +namespace { + +class MockReceiverObserver : public ReceiverObserver { public: ~MockReceiverObserver() override = default; @@ -37,14 +40,7 @@ class MockReceiverObserver final : public ReceiverObserver { void(const std::string&, const std::string&)); }; -class NullObserver final : public ProtocolConnectionServiceObserver { - public: - ~NullObserver() override = default; - void OnRunning() override {} - void OnStopped() override {} - void OnMetrics(const NetworkMetrics& metrics) override {} - void OnError(const Error& error) override {} -}; +} // namespace class UrlAvailabilityRequesterTest : public Test { public: @@ -367,12 +363,15 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserverUrls) { EXPECT_CALL(mock_observer1, OnReceiverUnavailable(url1_, service_id_)) .Times(0); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); MockReceiverObserver mock_observer2; + EXPECT_CALL(mock_observer2, OnReceiverAvailable(url1_, service_id_)); listener_.AddObserver({url1_, url2_}, &mock_observer2); ExpectStreamMessage(&mock_callback_, &request); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer2); EXPECT_EQ(std::vector{url2_}, request.urls); @@ -385,6 +384,8 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserverUrls) { EXPECT_CALL(mock_observer2, OnReceiverAvailable(_, service_id_)).Times(0); EXPECT_CALL(mock_observer2, OnReceiverUnavailable(url2_, service_id_)); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); + Mock::VerifyAndClearExpectations(&mock_observer2); SendAvailabilityEvent( url1_watch_id, @@ -395,6 +396,8 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserverUrls) { .Times(0); EXPECT_CALL(mock_observer2, OnReceiverUnavailable(url1_, service_id_)); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); + Mock::VerifyAndClearExpectations(&mock_observer2); } TEST_F(UrlAvailabilityRequesterTest, RemoveObserver) { @@ -421,12 +424,15 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserver) { EXPECT_CALL(mock_observer1, OnReceiverUnavailable(url1_, service_id_)) .Times(0); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); MockReceiverObserver mock_observer2; + EXPECT_CALL(mock_observer2, OnReceiverAvailable(url1_, service_id_)); listener_.AddObserver({url1_, url2_}, &mock_observer2); ExpectStreamMessage(&mock_callback_, &request); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer2); uint64_t url2_watch_id = request.watch_id; EXPECT_EQ(std::vector{url2_}, request.urls); @@ -440,6 +446,8 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserver) { EXPECT_CALL(mock_observer2, OnReceiverAvailable(_, service_id_)).Times(0); EXPECT_CALL(mock_observer2, OnReceiverUnavailable(url2_, service_id_)); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); + Mock::VerifyAndClearExpectations(&mock_observer2); SendAvailabilityEvent( url1_watch_id, @@ -450,6 +458,8 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserver) { .Times(0); EXPECT_CALL(mock_observer2, OnReceiverUnavailable(url1_, service_id_)); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); + Mock::VerifyAndClearExpectations(&mock_observer2); listener_.RemoveObserver(&mock_observer2); @@ -466,6 +476,8 @@ TEST_F(UrlAvailabilityRequesterTest, RemoveObserver) { EXPECT_CALL(mock_observer1, OnReceiverUnavailable(_, service_id_)).Times(0); EXPECT_CALL(mock_observer2, OnReceiverUnavailable(_, service_id_)).Times(0); quic_bridge_.RunTasksUntilIdle(); + Mock::VerifyAndClearExpectations(&mock_observer1); + Mock::VerifyAndClearExpectations(&mock_observer2); } TEST_F(UrlAvailabilityRequesterTest, EventUpdate) { -- cgit v1.2.3