From 3edd46ec8c0810e4b345db8fbc12b7aaa4c6aad8 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Tue, 17 Oct 2023 18:16:06 -0400 Subject: [PATCH] ceftests: Simplify completion for tests that don't just create browsers Some test cases don't create browsers at all, or require additional signals such as request context destruction. To test: Run `ceftests --gtest_filter=URLRequestTest.*` --- .../message_router_multi_query_unittest.cc | 11 +- .../resource_request_handler_unittest.cc | 21 +-- tests/ceftests/run_all_unittests.cc | 4 +- tests/ceftests/test_handler.cc | 136 +++++++++++++----- tests/ceftests/test_handler.h | 72 ++++++---- tests/ceftests/urlrequest_unittest.cc | 40 ++---- 6 files changed, 181 insertions(+), 103 deletions(-) diff --git a/tests/ceftests/message_router_multi_query_unittest.cc b/tests/ceftests/message_router_multi_query_unittest.cc index 1bb39c18e..e9c1f10cb 100644 --- a/tests/ceftests/message_router_multi_query_unittest.cc +++ b/tests/ceftests/message_router_multi_query_unittest.cc @@ -957,7 +957,7 @@ class MultiQuerySingleFrameTestHandler : public SingleLoadTestHandler, AssertQueryCount(nullptr, nullptr, 0); } else if (cancel_type_ == CANCEL_BY_CLOSING_BROWSER) { // Change the expected behavior in the handler. - SetSignalCompletionWhenAllBrowsersClose(false); + SetSignalTestCompletionCount(1U); CloseBrowser(GetBrowser(), false); } } @@ -971,11 +971,12 @@ class MultiQuerySingleFrameTestHandler : public SingleLoadTestHandler, DestroyTest(); - if (!SignalCompletionWhenAllBrowsersClose()) { + if (!AllowTestCompletionWhenAllBrowsersClose()) { // Complete asynchronously so the call stack has a chance to unwind. - CefPostTask(TID_UI, - base::BindOnce( - &MultiQuerySingleFrameTestHandler::TestComplete, this)); + CefPostTask( + TID_UI, + base::BindOnce( + &MultiQuerySingleFrameTestHandler::SignalTestCompletion, this)); } } diff --git a/tests/ceftests/resource_request_handler_unittest.cc b/tests/ceftests/resource_request_handler_unittest.cc index 72ccb885a..9212cee74 100644 --- a/tests/ceftests/resource_request_handler_unittest.cc +++ b/tests/ceftests/resource_request_handler_unittest.cc @@ -500,7 +500,7 @@ class BasicResponseTest : public TestHandler { TestHandler::OnAfterCreated(browser); if (mode_ == ABORT_AFTER_CREATED) { - SetSignalCompletionWhenAllBrowsersClose(false); + SetSignalTestCompletionCount(1U); CloseBrowser(browser, false); } } @@ -547,7 +547,7 @@ class BasicResponseTest : public TestHandler { VerifyState(kOnBeforeBrowse, request, nullptr); if (mode_ == ABORT_BEFORE_BROWSE) { - SetSignalCompletionWhenAllBrowsersClose(false); + SetSignalTestCompletionCount(1U); CloseBrowser(browser, false); } @@ -994,10 +994,10 @@ class BasicResponseTest : public TestHandler { TestHandler::DestroyTest(); - if (!SignalCompletionWhenAllBrowsersClose()) { + if (!AllowTestCompletionWhenAllBrowsersClose()) { // Complete asynchronously so the call stack has a chance to unwind. - CefPostTask(TID_UI, - base::BindOnce(&BasicResponseTest::TestComplete, this)); + CefPostTask(TID_UI, base::BindOnce( + &BasicResponseTest::SignalTestCompletion, this)); } } @@ -1337,7 +1337,7 @@ class BasicResponseTest : public TestHandler { void CloseBrowserAsync() { EXPECT_TRUE(IsIncomplete()); - SetSignalCompletionWhenAllBrowsersClose(false); + SetSignalTestCompletionCount(1U); CefPostDelayedTask( TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false), 100); @@ -2131,10 +2131,11 @@ class SubresourceResponseTest : public RoutingTestHandler { TestHandler::DestroyTest(); - if (!SignalCompletionWhenAllBrowsersClose()) { + if (!AllowTestCompletionWhenAllBrowsersClose()) { // Complete asynchronously so the call stack has a chance to unwind. - CefPostTask(TID_UI, - base::BindOnce(&SubresourceResponseTest::TestComplete, this)); + CefPostTask( + TID_UI, + base::BindOnce(&SubresourceResponseTest::SignalTestCompletion, this)); } } @@ -2544,7 +2545,7 @@ class SubresourceResponseTest : public RoutingTestHandler { void CloseBrowserAsync() { EXPECT_TRUE(IsIncomplete()); - SetSignalCompletionWhenAllBrowsersClose(false); + SetSignalTestCompletionCount(1U); CefPostDelayedTask( TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false), 100); diff --git a/tests/ceftests/run_all_unittests.cc b/tests/ceftests/run_all_unittests.cc index 99f52df41..dd37370f8 100644 --- a/tests/ceftests/run_all_unittests.cc +++ b/tests/ceftests/run_all_unittests.cc @@ -74,8 +74,8 @@ void RunTestsOnTestThread() { // Run the test suite. CefTestSuite::GetInstance()->Run(); - // Wait for all browsers to exit. - while (TestHandler::HasBrowser()) { + // Wait for all TestHandlers to be destroyed. + while (TestHandler::HasTestHandler()) { sleep(100); } diff --git a/tests/ceftests/test_handler.cc b/tests/ceftests/test_handler.cc index b681b19e8..2afe4262f 100644 --- a/tests/ceftests/test_handler.cc +++ b/tests/ceftests/test_handler.cc @@ -233,10 +233,13 @@ void TestHandler::UIThreadHelper::TaskHelper(base::OnceClosure task) { // TestHandler -int TestHandler::browser_count_ = 0; +// static +std::atomic TestHandler::test_handler_count_{0U}; TestHandler::TestHandler(CompletionState* completion_state) : debug_string_prefix_(MakeDebugStringPrefix()) { + test_handler_count_++; + if (completion_state) { completion_state_ = completion_state; completion_state_owned_ = false; @@ -254,7 +257,6 @@ TestHandler::~TestHandler() { EXPECT_FALSE(destroy_test_called_); } EXPECT_TRUE(browser_map_.empty()); - EXPECT_EQ(0U, window_count_); EXPECT_TRUE(browser_status_map_.empty()); if (completion_state_owned_) { @@ -264,6 +266,8 @@ TestHandler::~TestHandler() { if (destroy_event_) { destroy_event_->Signal(); } + + test_handler_count_--; } void TestHandler::OnAfterCreated(CefRefPtr browser) { @@ -300,26 +304,25 @@ void TestHandler::OnBeforeClose(CefRefPtr browser) { void TestHandler::OnWindowCreated(int browser_id) { CHECK(UseViews()); EXPECT_UI_THREAD(); - window_count_++; OnCreated(browser_id, NT_WINDOW); } void TestHandler::OnWindowDestroyed(int browser_id) { CHECK(UseViews()); EXPECT_UI_THREAD(); - window_count_--; OnClosed(browser_id, NT_WINDOW); } void TestHandler::OnCreated(int browser_id, NotifyType type) { - bool creation_complete = false; - auto& browser_status = browser_status_map_[browser_id]; EXPECT_FALSE(browser_status.got_created[type]) << "Duplicate call to OnCreated(" << browser_id << ", " << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << ")"; browser_status.got_created[type].yes(); +#if VERBOSE_DEBUGGING + bool creation_complete = false; + // When using Views, wait for both Browser and Window notifications. if (UseViews()) { creation_complete = browser_status.got_created[NT_BROWSER] && @@ -328,15 +331,10 @@ void TestHandler::OnCreated(int browser_id, NotifyType type) { creation_complete = browser_status.got_created[NT_BROWSER]; } -#if VERBOSE_DEBUGGING LOG(INFO) << debug_string_prefix_ << browser_id << ": OnCreated type=" << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << " creation_complete=" << creation_complete; -#endif - - if (creation_complete) { - browser_count_++; - } +#endif // VERBOSE_DEBUGGING } void TestHandler::OnClosed(int browser_id, NotifyType type) { @@ -356,29 +354,23 @@ void TestHandler::OnClosed(int browser_id, NotifyType type) { close_complete = browser_status.got_closed[NT_BROWSER]; } - // Test is complete if no Browsers/Windows are remaining. - const bool test_complete = - close_complete && - (UseViews() ? window_count_ == 0 : browser_map_.empty()); + if (close_complete) { + browser_status_map_.erase(browser_id); + } + + // Test may be complete if no Browsers/Windows are remaining. + const bool all_browsers_closed = AllBrowsersClosed(); #if VERBOSE_DEBUGGING LOG(INFO) << debug_string_prefix_ << browser_id << ": OnClosed type=" << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << " close_complete=" << close_complete - << " test_complete=" << test_complete; + << " all_browsers_closed=" << all_browsers_closed; #endif - if (close_complete) { - browser_status_map_.erase(browser_id); - } - - if (test_complete && signal_completion_when_all_browsers_close_) { - // Signal that the test is now complete. May result in |this| being deleted. - TestComplete(); - } - - if (close_complete) { - browser_count_--; + if (all_browsers_closed) { + // May result in |this| being deleted. + MaybeTestComplete(); } } @@ -491,18 +483,30 @@ void TestHandler::DestroyTest() { CloseBrowser(it->second, false); } } - - if (ui_thread_helper_.get()) { - ui_thread_helper_.reset(nullptr); - } } void TestHandler::OnTestTimeout(int timeout_ms, bool treat_as_error) { EXPECT_UI_THREAD(); + + EXPECT_FALSE(test_timeout_called_); + test_timeout_called_ = true; + if (treat_as_error) { EXPECT_TRUE(false) << "Test timed out after " << timeout_ms << "ms"; } + + EXPECT_FALSE(AllBrowsersClosed() && AllowTestCompletionWhenAllBrowsersClose()) + << "Test timed out unexpectedly; should be complete"; + + // Close any remaining browsers. DestroyTest(); + + // Reset signal completion count. + if (signal_completion_count_ > 0) { + signal_completion_count_ = 0; + // May result in |this| being deleted. + MaybeTestComplete(); + } } void TestHandler::CreateBrowser(const CefString& url, @@ -606,17 +610,79 @@ void TestHandler::SetTestTimeout(int timeout_ms, bool treat_as_error) { *timeout); } -void TestHandler::TestComplete() { +void TestHandler::SetSignalTestCompletionCount(size_t count) { +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ + << "SetSignalTestCompletionCount count=" << count; +#endif + signal_completion_count_ = count; +} + +void TestHandler::SignalTestCompletion() { if (!CefCurrentlyOn(TID_UI)) { - CefPostTask(TID_UI, base::BindOnce(&TestHandler::TestComplete, this)); + CefPostTask(TID_UI, + base::BindOnce(&TestHandler::SignalTestCompletion, this)); return; } + if (test_timeout_called_) { + // Ignore any signals that arrive after test timeout. + return; + } + + CHECK_GT(signal_completion_count_, 0U); + signal_completion_count_--; + +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ + << "SignalTestComplete remaining=" << signal_completion_count_; +#endif + + if (signal_completion_count_ == 0) { + // May result in |this| being deleted. + MaybeTestComplete(); + } +} + +bool TestHandler::AllowTestCompletionWhenAllBrowsersClose() const { + EXPECT_UI_THREAD(); + return signal_completion_count_ == 0U; +} + +bool TestHandler::AllBrowsersClosed() const { + EXPECT_UI_THREAD(); + return browser_status_map_.empty(); +} + +void TestHandler::MaybeTestComplete() { + EXPECT_UI_THREAD(); + + const bool all_browsers_closed = AllBrowsersClosed(); + const bool allow_test_completion = AllowTestCompletionWhenAllBrowsersClose(); + +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ + << "MaybeTestComplete all_browsers_closed=" << all_browsers_closed + << " allow_test_completion=" << allow_test_completion; +#endif + + if (all_browsers_closed && allow_test_completion) { + TestComplete(); + } +} + +void TestHandler::TestComplete() { + EXPECT_UI_THREAD(); + EXPECT_TRUE(AllBrowsersClosed()); + EXPECT_TRUE(AllowTestCompletionWhenAllBrowsersClose()); + #if VERBOSE_DEBUGGING LOG(INFO) << debug_string_prefix_ << "TestComplete"; #endif - EXPECT_TRUE(browser_map_.empty()); + // Cancel any pending tasks posted via UIThreadHelper. + ui_thread_helper_.reset(); + completion_state_->TestComplete(); } diff --git a/tests/ceftests/test_handler.h b/tests/ceftests/test_handler.h index 9613176f4..dbdeb1c50 100644 --- a/tests/ceftests/test_handler.h +++ b/tests/ceftests/test_handler.h @@ -6,6 +6,7 @@ #define CEF_TESTS_CEFTESTS_TEST_HANDLER_H_ #pragma once +#include #include #include #include @@ -210,8 +211,10 @@ class TestHandler : public CefClient, void OnWindowCreated(int browser_id); void OnWindowDestroyed(int browser_id); - // Returns true if a browser currently exists. - static bool HasBrowser() { return browser_count_ > 0; } + // Returns true if a TestHandler currently exists. + static bool HasTestHandler() { + return test_handler_count_.load(std::memory_order_relaxed) > 0U; + } std::string debug_string_prefix() const { return debug_string_prefix_; } @@ -220,11 +223,8 @@ class TestHandler : public CefClient, // Collection. virtual void SetupComplete(); - // Close any existing non-popup browsers. Test completion will be signaled - // once all the browsers have closed if - // |signal_completion_when_all_browsers_close_| is true (default value). - // If no browsers exist then this method will do nothing and - // TestComplete() must be called manually. + // Close any remaining browsers. This may result in a call to TestComplete(), + // depending on the configuration of SetSignalCompletionCount(). virtual void DestroyTest(); // Called on the UI thread if the test times out as a result of calling @@ -245,27 +245,47 @@ class TestHandler : public CefClient, void ClearResources(); - void SetSignalCompletionWhenAllBrowsersClose(bool val) { - signal_completion_when_all_browsers_close_ = val; - } - bool SignalCompletionWhenAllBrowsersClose() const { - return signal_completion_when_all_browsers_close_; - } + // Specify the number of times that SignalTestCompletion() needs to be + // explicitly called for test completion. Must be configured during test + // initialization before any browsers are created. + // - If the test creates browsers and does not explicitly call + // SignalTestCompletion() then the default value (0) can be used. + // - If the test creates browsers and explicitly calls SignalTestCompletion() + // then set a value >= 1. + // - If the test does not create browsers then it must explicitly call + // SignalTestCompletion() and set a value >= 1. + void SetSignalTestCompletionCount(size_t count); + + // Explicitly signal test completion a single time. Used in combination with + // SetSignalTestCompletionCount(). Results in a call to TestComplete() if + // all browsers have closed and this method has been called the expected + // number of times. + void SignalTestCompletion(); + + // Reuturns true if SignalTestCompletion() has been called the necessary + // number of times (may be 0), in which case TestComplete() will be called + // automatically when all browsers have closed. Must be called on the + // UI thread. + bool AllowTestCompletionWhenAllBrowsersClose() const; + + // Returns true if all browsers have closed. Must be called on the UI + // thread. + bool AllBrowsersClosed() const; // Call OnTestTimeout() after the specified amount of time. void SetTestTimeout(int timeout_ms = 5000, bool treat_as_error = true); - // Signal that the test is complete. This will be called automatically when - // all existing non-popup browsers are closed if - // |signal_completion_when_all_browsers_close_| is true (default value). It - // is an error to call this method before all browsers have closed. - void TestComplete(); - // Returns the single UIThreadHelper instance, creating it if necessary. Must // be called on the UI thread. UIThreadHelper* GetUIThreadHelper(); private: + void MaybeTestComplete(); + + // Complete the test. It is an error to call this method before all browsers + // have closed. + void TestComplete(); + enum NotifyType { NT_BROWSER, NT_WINDOW, @@ -283,9 +303,6 @@ class TestHandler : public CefClient, // Map of browser ID to browser object. Only accessed on the UI thread. BrowserMap browser_map_; - // Count of window objects. Only accessed on the UI thread. - size_t window_count_ = 0; - struct NotifyStatus { // Keyed by NotifyType. TrackCallback got_created[2]; @@ -305,19 +322,22 @@ class TestHandler : public CefClient, typedef std::map ResourceMap; ResourceMap resource_map_; - // If true test completion will be signaled when all browsers have closed. - bool signal_completion_when_all_browsers_close_ = true; + // Number of times that SignalTestCompletion() must be called. + size_t signal_completion_count_ = 0U; CefRefPtr destroy_event_; + // Tracks whether OnTestTimeout() has been called. + bool test_timeout_called_ = false; + // Tracks whether DestroyTest() is expected or has been called. bool destroy_test_expected_ = true; bool destroy_test_called_ = false; std::unique_ptr ui_thread_helper_; - // Used to track the number of currently existing browser windows. - static int browser_count_; + // Used to track the number of currently existing TestHandlers. + static std::atomic test_handler_count_; DISALLOW_COPY_AND_ASSIGN(TestHandler); }; diff --git a/tests/ceftests/urlrequest_unittest.cc b/tests/ceftests/urlrequest_unittest.cc index f4d76a753..a3c033a9f 100644 --- a/tests/ceftests/urlrequest_unittest.cc +++ b/tests/ceftests/urlrequest_unittest.cc @@ -2628,6 +2628,18 @@ class RequestTestHandler : public TestHandler { base::BindOnce(&RequestTestHandler::OnIncompleteRequest, this)); test_runner_->Initialize(); + // Configure the number of times that SignalTestCompletion() will be called. + // We need to call it at least 1 time if we don't create a browser. + size_t completion_count = test_frame_method_ ? 0U : 1U; + if (context_mode_ != CONTEXT_GLOBAL) { + // Don't end the test until the temporary request context has been + // destroyed. + completion_count++; + } + if (completion_count > 0U) { + SetSignalTestCompletionCount(completion_count); + } + // Get or create the request context. if (context_mode_ == CONTEXT_GLOBAL) { CefRefPtr request_context = @@ -2637,10 +2649,6 @@ class RequestTestHandler : public TestHandler { PreSetupComplete(); } else { - // Don't end the test until the temporary request context has been - // destroyed. - SetSignalCompletionWhenAllBrowsersClose(false); - CefRequestContextSettings settings; if (context_mode_ == CONTEXT_ONDISK) { @@ -2768,7 +2776,6 @@ class RequestTestHandler : public TestHandler { // TestComplete will eventually be called from DestroyTest instead of being // triggered by browser destruction. - SetSignalCompletionWhenAllBrowsersClose(false); CefPostDelayedTask( TID_UI, base::BindOnce(&TestHandler::CloseBrowser, GetBrowser(), false), 1000); @@ -2826,24 +2833,11 @@ class RequestTestHandler : public TestHandler { TestHandler::DestroyTest(); - // For non-global contexts OnTestComplete() will be called when the - // RequestContextHandler is destroyed. - bool call_test_complete = false; - if (context_mode_ == CONTEXT_GLOBAL) { - if (!test_frame_method_) { - // These tests don't create a browser that would signal implicitly. - call_test_complete = true; - } else if (!SignalCompletionWhenAllBrowsersClose()) { - // These tests close the browser to terminate in-progress requests - // before test completion. - call_test_complete = true; - } - } - // Release references to the context and handler. test_runner_->Destroy(); - if (call_test_complete) { + // These tests don't create a browser that would signal implicitly. + if (!test_frame_method_) { OnTestComplete(); } } @@ -2855,15 +2849,12 @@ class RequestTestHandler : public TestHandler { return; } - EXPECT_FALSE(got_on_test_complete_); - got_on_test_complete_.yes(); - if (!context_tmpdir_.IsEmpty()) { // Temp directory will be deleted on application shutdown. context_tmpdir_.Take(); } - TestComplete(); + SignalTestCompletion(); } private: @@ -2904,7 +2895,6 @@ class RequestTestHandler : public TestHandler { public: int auth_credentials_ct_ = 0; - TrackCallback got_on_test_complete_; IMPLEMENT_REFCOUNTING(RequestTestHandler); };