From ec1e1cfbfc093c84af08d7cbe1d2e5b963cf5afa Mon Sep 17 00:00:00 2001 From: Peter Kasting Date: Thu, 26 Mar 2020 23:11:18 +0000 Subject: [PATCH] Reorder AshTestHelper setup/teardown. This is in preparation for making AshTestHelper an AuraTestHelper subclass. The primary purpose here is to move the parts of setup/ teardown that will be handled by the AuraTestHelper to the start/end of the ash setup/teardown phases, respectively, so that moving them into AuraTestHelper will not change the relative order of anything. The secondary purpose, subject to the above, is to better group related functionality and add a few comments about ordering requirements. I didn't aim for perfection, just something better. This is split into its own CL since reordering setup/teardown is risky; there are many implicit dependencies. There are some trivial changes: * Removed a null-check of the cursor manager that was only needed for MASH. * Explicitly reset all unique_ptrs during TearDown() rather than waiting until destruction for a few. * Removed CHECK(!::wm::CaptureController::Get()) during teardown; it's not clear to me why we CHECK this but none of the other many bits of state destruction on teardown. Otherwise, should be no functional change. Bug: none Change-Id: Ief088e58606b83e127e93daad0a60f90601b9866 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2121085 Auto-Submit: Peter Kasting Commit-Queue: James Cook Reviewed-by: James Cook Cr-Commit-Position: refs/heads/master@{#753811} --- ash/test/ash_test_helper.cc | 245 ++++++++++++++++++------------------ ash/test/ash_test_helper.h | 29 ++--- 2 files changed, 131 insertions(+), 143 deletions(-) diff --git a/ash/test/ash_test_helper.cc b/ash/test/ash_test_helper.cc index 39713e0894bd3c..2bd6ae40c4f4b1 100644 --- a/ash/test/ash_test_helper.cc +++ b/ash/test/ash_test_helper.cc @@ -70,16 +70,55 @@ AshTestHelper::InitParams::InitParams() = default; AshTestHelper::InitParams::InitParams(InitParams&&) = default; AshTestHelper::InitParams::~InitParams() = default; -AshTestHelper::AshTestHelper() - : command_line_(std::make_unique()) {} +AshTestHelper::AshTestHelper() = default; AshTestHelper::~AshTestHelper() { - // Ensure the next test starts with a null display::Screen. Done here because - // some tests use Screen after TearDown(). + // Ensure the next test starts with a null display::Screen. This must be done + // here instead of in TearDown() since some tests test access to the Screen + // after the shell shuts down (which they use TearDown() to trigger). ScreenAsh::DeleteScreenForShutdown(); } void AshTestHelper::SetUp(InitParams init_params) { + // Aura-general setup ------------------------------------------------------- + + wm_state_ = std::make_unique<::wm::WMState>(); + + if (init_params.config_type != kShell) { + ui::test::EnableTestConfigForPlatformWindows(); + ui::InitializeInputMethodForTesting(); + } + + ui::test::EventGeneratorDelegate::SetFactoryFunction( + base::BindRepeating(&aura::test::EventGeneratorDelegateAura::Create)); + + if (init_params.config_type == kUnitTest) { + zero_duration_mode_.reset(new ui::ScopedAnimationDurationScaleMode( + ui::ScopedAnimationDurationScaleMode::ZERO_DURATION)); + } + + if (!init_params.context_factory) { + context_factories_ = std::make_unique(false); + init_params.context_factory = context_factories_->GetContextFactory(); + } + + // Reset aura::Env to eliminate test dependency (https://crbug.com/586514). + aura::test::EnvTestHelper env_helper(aura::Env::GetInstance()); + env_helper.ResetEnvForTesting(); + env_helper.SetInputStateLookup(std::unique_ptr()); + + // Ash-specific setup ------------------------------------------------------- + + command_line_ = std::make_unique(); + statistics_provider_ = + std::make_unique(); + prefs_provider_ = std::make_unique(); + notifier_settings_controller_ = + std::make_unique(); + assistant_service_ = std::make_unique(); + system_tray_client_ = std::make_unique(); + photo_controller_ = std::make_unique(); + // TODO(jamescook): Can we do this without changing command line? // Use the origin (1,1) so that it doesn't over // lap with the native mouse cursor. @@ -91,156 +130,108 @@ void AshTestHelper::SetUp(InitParams init_params) { ::switches::kHostWindowBounds, "10+10-800x600"); } - // Pre shell creation config init. - switch (init_params.config_type) { - case kUnitTest: - // Default for unit tests but not for perf tests. - zero_duration_mode_.reset(new ui::ScopedAnimationDurationScaleMode( - ui::ScopedAnimationDurationScaleMode::ZERO_DURATION)); - TabletModeController::SetUseScreenshotForTest(false); - FALLTHROUGH; - case kPerfTest: - // Default for both unit and perf tests. - ui::test::EnableTestConfigForPlatformWindows(); - display::ResetDisplayIdForTest(); - ui::InitializeInputMethodForTesting(); - break; - case kShell: - break; - } + if (init_params.config_type == kUnitTest) + TabletModeController::SetUseScreenshotForTest(false); - statistics_provider_ = - std::make_unique(); + if (init_params.config_type != kShell) + display::ResetDisplayIdForTest(); - ui::test::EventGeneratorDelegate::SetFactoryFunction( - base::BindRepeating(&aura::test::EventGeneratorDelegateAura::Create)); + chromeos::CrasAudioClient::InitializeFake(); + // Create CrasAudioHandler for testing since g_browser_process is not + // created in AshTestBase tests. + chromeos::CrasAudioHandler::InitializeForTesting(); - wm_state_ = std::make_unique<::wm::WMState>(); - // Only create a ViewsDelegate if the test didn't create one already. - if (!views::ViewsDelegate::GetInstance()) - test_views_delegate_ = std::make_unique(); + // Reset the global state for the cursor manager. This includes the + // last cursor visibility state, etc. + ::wm::CursorManager::ResetCursorVisibilityStateForTest(); if (!bluez::BluezDBusManager::IsInitialized()) { bluez::BluezDBusManager::InitializeFake(); bluez_dbus_manager_initialized_ = true; } - if (!chromeos::PowerManagerClient::Get()) chromeos::PowerManagerClient::InitializeFake(); - if (!chromeos::PowerPolicyController::IsInitialized()) { chromeos::PowerPolicyController::Initialize( chromeos::PowerManagerClient::Get()); power_policy_controller_initialized_ = true; } - - chromeos::CrasAudioClient::InitializeFake(); - // Create CrasAudioHandler for testing since g_browser_process is not - // created in AshTestBase tests. - chromeos::CrasAudioHandler::InitializeForTesting(); - - // Reset the global state for the cursor manager. This includes the - // last cursor visibility state, etc. - ::wm::CursorManager::ResetCursorVisibilityStateForTest(); + if (!NewWindowDelegate::GetInstance()) + new_window_delegate_ = std::make_unique(); + if (!views::ViewsDelegate::GetInstance()) + test_views_delegate_ = std::make_unique(); ShellInitParams shell_init_params; shell_init_params.delegate = std::move(init_params.delegate); if (!shell_init_params.delegate) shell_init_params.delegate = std::make_unique(); shell_init_params.context_factory = init_params.context_factory; - if (!shell_init_params.context_factory) { - context_factories_ = std::make_unique(false); - shell_init_params.context_factory = context_factories_->GetContextFactory(); - } shell_init_params.local_state = init_params.local_state; shell_init_params.keyboard_ui_factory = std::make_unique(); Shell::CreateInstance(std::move(shell_init_params)); - - // Reset aura::Env to eliminate test dependency (https://crbug.com/586514). - aura::test::EnvTestHelper env_helper(aura::Env::GetInstance()); - env_helper.ResetEnvForTesting(); - - env_helper.SetInputStateLookup(std::unique_ptr()); - Shell* shell = Shell::Get(); // Cursor is visible by default in tests. - // CursorManager is null on MASH. - if (shell->cursor_manager()) - shell->cursor_manager()->ShowCursor(); - - prefs_provider_ = std::make_unique(); - session_controller_client_.reset(new TestSessionControllerClient( - shell->session_controller(), prefs_provider_.get())); - session_controller_client_->InitializeAndSetClient(); - - notifier_settings_controller_ = - std::make_unique(); + shell->cursor_manager()->ShowCursor(); - assistant_service_ = std::make_unique(); shell->assistant_controller()->SetAssistant( assistant_service_->CreateRemoteAndBind()); - system_tray_client_ = std::make_unique(); shell->system_tray_model()->SetClient(system_tray_client_.get()); - photo_controller_ = std::make_unique(); - + session_controller_client_.reset(new TestSessionControllerClient( + shell->session_controller(), prefs_provider_.get())); + session_controller_client_->InitializeAndSetClient(); if (init_params.start_session) session_controller_client_->CreatePredefinedUserSessions(1); + // Requires the AppListController the Shell creates. app_list_test_helper_ = std::make_unique(); - if (!NewWindowDelegate::GetInstance()) - new_window_delegate_ = std::make_unique(); - - // Post shell creation config init. - switch (init_params.config_type) { - case kUnitTest: - // Tests that change the display configuration generally don't care about - // the notifications and the popup UI can interfere with things like - // cursors. - shell->screen_layout_observer()->set_show_notifications_for_testing( - false); - - // Disable display change animations in unit tests. - DisplayConfigurationControllerTestApi( - shell->display_configuration_controller()) - .SetDisplayAnimator(false); - - // Remove the app dragging animations delay for testing purposes. - shell->overview_controller()->set_delayed_animation_task_delay_for_test( - base::TimeDelta()); - // Tests expect empty wallpaper. - shell->wallpaper_controller()->CreateEmptyWallpaperForTesting(); - - FALLTHROUGH; - case kPerfTest: - // Don't change the display size due to host size resize. - display::test::DisplayManagerTestApi(shell->display_manager()) - .DisableChangeDisplayUponHostResize(); - - // Create the test keyboard controller observer to respond to - // OnLoadKeyboardContentsRequested(). - test_keyboard_controller_observer_ = - std::make_unique( - shell->keyboard_controller()); - break; - case kShell: - shell->wallpaper_controller()->ShowDefaultWallpaperForTesting(); - break; + if (init_params.config_type == kShell) { + shell->wallpaper_controller()->ShowDefaultWallpaperForTesting(); + return; } + + // Don't change the display size due to host size resize. + display::test::DisplayManagerTestApi(shell->display_manager()) + .DisableChangeDisplayUponHostResize(); + + // Create the test keyboard controller observer to respond to + // OnLoadKeyboardContentsRequested(). + test_keyboard_controller_observer_ = + std::make_unique( + shell->keyboard_controller()); + + if (init_params.config_type != kUnitTest) + return; + + // Tests that change the display configuration generally don't care about the + // notifications and the popup UI can interfere with things like cursors. + shell->screen_layout_observer()->set_show_notifications_for_testing(false); + + // Disable display change animations in unit tests. + DisplayConfigurationControllerTestApi( + shell->display_configuration_controller()) + .SetDisplayAnimator(false); + + // Remove the app dragging animations delay for testing purposes. + shell->overview_controller()->set_delayed_animation_task_delay_for_test( + base::TimeDelta()); + + // Tests expect empty wallpaper. + shell->wallpaper_controller()->CreateEmptyWallpaperForTesting(); } void AshTestHelper::TearDown() { + // Ash-specific teardown ---------------------------------------------------- + + // The AppListTestHelper holds a pointer to the AppListController the Shell + // owns, so shut the test helper down first. app_list_test_helper_.reset(); Shell::DeleteInstance(); - new_window_delegate_.reset(); - - // Needs to be reset after Shell::Get()->keyboard_controller() is deleted. - test_keyboard_controller_observer_.reset(); // Suspend the tear down until all resources are returned via // CompositorFrameSinkClient::ReclaimResources() @@ -249,6 +240,8 @@ void AshTestHelper::TearDown() { chromeos::CrasAudioHandler::Shutdown(); chromeos::CrasAudioClient::Shutdown(); + // The PowerPolicyController holds a pointer to the PowerManagementClient, so + // shut the controller down first. if (power_policy_controller_initialized_) { chromeos::PowerPolicyController::Shutdown(); power_policy_controller_initialized_ = false; @@ -256,13 +249,29 @@ void AshTestHelper::TearDown() { chromeos::PowerManagerClient::Shutdown(); + TabletModeController::SetUseScreenshotForTest(true); + + // Destroy all owned objects to prevent tests from depending on their state + // after this returns. + test_keyboard_controller_observer_.reset(); + test_views_delegate_.reset(); + new_window_delegate_.reset(); if (bluez_dbus_manager_initialized_) { device::BluetoothAdapterFactory::Shutdown(); bluez::BluezDBusManager::Shutdown(); bluez_dbus_manager_initialized_ = false; } + photo_controller_.reset(); + system_tray_client_.reset(); + assistant_service_.reset(); + notifier_settings_controller_.reset(); + prefs_provider_.reset(); + statistics_provider_.reset(); + command_line_.reset(); - context_factories_.reset(); + // Aura-general teardown ---------------------------------------------------- + + ui::ShutdownInputMethodForTesting(); // Context factory referenced by Env is now destroyed. Reset Env's members in // case some other test tries to use it. This matters if someone else created @@ -270,24 +279,12 @@ void AshTestHelper::TearDown() { if (aura::Env::HasInstance()) aura::Env::GetInstance()->set_context_factory(nullptr); - ui::ShutdownInputMethodForTesting(); - zero_duration_mode_.reset(); - - test_views_delegate_.reset(); - wm_state_.reset(); - - command_line_.reset(); - - display::Display::ResetForceDeviceScaleFactorForTesting(); - - CHECK(!::wm::CaptureController::Get()); - ui::test::EventGeneratorDelegate::SetFactoryFunction( ui::test::EventGeneratorDelegate::FactoryFunction()); - statistics_provider_.reset(); - - TabletModeController::SetUseScreenshotForTest(true); + context_factories_.reset(); + zero_duration_mode_.reset(); + wm_state_.reset(); } PrefService* AshTestHelper::GetLocalStatePrefService() { diff --git a/ash/test/ash_test_helper.h b/ash/test/ash_test_helper.h index 95524f1f31d79a..f0fed39ddccffa 100644 --- a/ash/test/ash_test_helper.h +++ b/ash/test/ash_test_helper.h @@ -135,32 +135,23 @@ class AshTestHelper { void reset_commandline() { command_line_.reset(); } private: + std::unique_ptr<::wm::WMState> wm_state_; + std::unique_ptr zero_duration_mode_; + std::unique_ptr context_factories_; + std::unique_ptr command_line_; std::unique_ptr statistics_provider_; - - std::unique_ptr zero_duration_mode_; - - std::unique_ptr<::wm::WMState> wm_state_; - std::unique_ptr test_views_delegate_; - - // Flags for whether various services were initialized here. - bool bluez_dbus_manager_initialized_ = false; - bool power_policy_controller_initialized_ = false; - - std::unique_ptr session_controller_client_; - std::unique_ptr notifier_settings_controller_; - std::unique_ptr system_tray_client_; std::unique_ptr prefs_provider_; + std::unique_ptr notifier_settings_controller_; std::unique_ptr assistant_service_; - std::unique_ptr context_factories_; + std::unique_ptr system_tray_client_; std::unique_ptr photo_controller_; - - std::unique_ptr command_line_; - std::unique_ptr app_list_test_helper_; - + bool bluez_dbus_manager_initialized_ = false; + bool power_policy_controller_initialized_ = false; std::unique_ptr new_window_delegate_; - + std::unique_ptr test_views_delegate_; + std::unique_ptr session_controller_client_; std::unique_ptr test_keyboard_controller_observer_;