From 4e60afdf2ad5688aca276a14effe59444023b07f Mon Sep 17 00:00:00 2001 From: David Worsham Date: Tue, 11 Aug 2020 17:31:25 -0700 Subject: [PATCH] Fix broken shell_unittests on Fuchsia (flutter/engine#20422) Fixes https://github.com/flutter/flutter/issues/53399 --- engine/src/flutter/fml/file.cc | 11 +++++- engine/src/flutter/shell/common/BUILD.gn | 18 ++++++--- .../flutter/shell/common/shell_unittests.cc | 38 ++++++++++++------- .../src/flutter/testing/fuchsia/run_tests.sh | 11 ++---- .../src/flutter/vulkan/vulkan_application.cc | 6 +-- 5 files changed, 52 insertions(+), 32 deletions(-) diff --git a/engine/src/flutter/fml/file.cc b/engine/src/flutter/fml/file.cc index 3d9e4397ee6..b96bfe63414 100644 --- a/engine/src/flutter/fml/file.cc +++ b/engine/src/flutter/fml/file.cc @@ -44,14 +44,21 @@ fml::UniqueFD CreateDirectory(const fml::UniqueFD& base_directory, return CreateDirectory(base_directory, components, permission, 0); } -ScopedTemporaryDirectory::ScopedTemporaryDirectory() { - path_ = CreateTemporaryDirectory(); +ScopedTemporaryDirectory::ScopedTemporaryDirectory() + : path_(CreateTemporaryDirectory()) { if (path_ != "") { dir_fd_ = OpenDirectory(path_.c_str(), false, FilePermission::kRead); } } ScopedTemporaryDirectory::~ScopedTemporaryDirectory() { + // POSIX requires the directory to be empty before UnlinkDirectory. + if (path_ != "") { + if (!RemoveFilesInDirectory(dir_fd_)) { + FML_LOG(ERROR) << "Could not clean directory: " << path_; + } + } + // Windows has to close UniqueFD first before UnlinkDirectory dir_fd_.reset(); if (path_ != "") { diff --git a/engine/src/flutter/shell/common/BUILD.gn b/engine/src/flutter/shell/common/BUILD.gn index 8ef3f1c3c5b..941db5a8a83 100644 --- a/engine/src/flutter/shell/common/BUILD.gn +++ b/engine/src/flutter/shell/common/BUILD.gn @@ -158,6 +158,14 @@ if (enable_unittests) { test_enable_metal = false } + config("test_enable_gl_config") { + defines = [ "SHELL_ENABLE_GL" ] + } + + config("test_enable_vulkan_config") { + defines = [ "SHELL_ENABLE_VULKAN" ] + } + shell_gpu_configuration("shell_unittests_gpu_configuration") { enable_software = test_enable_software enable_vulkan = test_enable_vulkan @@ -208,7 +216,7 @@ if (enable_unittests) { "//third_party/skia", ] - defines = [] + public_configs = [] # SwiftShader only supports x86/x64_64 if (target_cpu == "x86" || target_cpu == "x64") { @@ -218,9 +226,9 @@ if (enable_unittests) { "shell_test_platform_view_gl.h", ] - public_deps += [ "//flutter/testing:opengl" ] + public_configs += [ ":test_enable_gl_config" ] - defines += [ "SHELL_ENABLE_GL" ] + public_deps += [ "//flutter/testing:opengl" ] } } @@ -230,12 +238,12 @@ if (enable_unittests) { "shell_test_platform_view_vulkan.h", ] + public_configs += [ ":test_enable_vulkan_config" ] + public_deps += [ "//flutter/testing:vulkan", "//flutter/vulkan", ] - - defines += [ "SHELL_ENABLE_VULKAN" ] } public_deps_legacy_and_next = [ diff --git a/engine/src/flutter/shell/common/shell_unittests.cc b/engine/src/flutter/shell/common/shell_unittests.cc index f7e68679a9d..d1ce6ecfbaf 100644 --- a/engine/src/flutter/shell/common/shell_unittests.cc +++ b/engine/src/flutter/shell/common/shell_unittests.cc @@ -32,10 +32,14 @@ #include "flutter/shell/common/vsync_waiter_fallback.h" #include "flutter/shell/version/version.h" #include "flutter/testing/testing.h" -#include "rapidjson/writer.h" +#include "third_party/rapidjson/include/rapidjson/writer.h" #include "third_party/skia/include/core/SkPictureRecorder.h" #include "third_party/tonic/converter/dart_converter.h" +#ifdef SHELL_ENABLE_VULKAN +#include "flutter/vulkan/vulkan_application.h" // nogncheck +#endif + namespace flutter { namespace testing { @@ -576,16 +580,16 @@ TEST_F(ShellTest, ReportTimingsIsCalledSoonerInNonReleaseMode) { DestroyShell(std::move(shell)); fml::TimePoint finish = fml::TimePoint::Now(); - fml::TimeDelta ellapsed = finish - start; + fml::TimeDelta elapsed = finish - start; #if FLUTTER_RELEASE // Our batch time is 1000ms. Hopefully the 800ms limit is relaxed enough to // make it not too flaky. - ASSERT_TRUE(ellapsed >= fml::TimeDelta::FromMilliseconds(800)); + ASSERT_TRUE(elapsed >= fml::TimeDelta::FromMilliseconds(800)); #else // Our batch time is 100ms. Hopefully the 500ms limit is relaxed enough to // make it not too flaky. - ASSERT_TRUE(ellapsed <= fml::TimeDelta::FromMilliseconds(500)); + ASSERT_TRUE(elapsed <= fml::TimeDelta::FromMilliseconds(500)); #endif } @@ -797,8 +801,15 @@ TEST_F(ShellTest, SetResourceCacheSize) { RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + // The Vulkan and GL backends set different default values for the resource + // cache size. +#ifdef SHELL_ENABLE_VULKAN + EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), + vulkan::kGrCacheMaxByteSize); +#else EXPECT_EQ(GetRasterizerResourceCacheBytesSync(*shell), static_cast(24 * (1 << 20))); +#endif fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), [&shell]() { @@ -1231,15 +1242,17 @@ TEST_F(ShellTest, CanDecompressImageFromAsset) { } TEST_F(ShellTest, OnServiceProtocolGetSkSLsWorks) { + fml::ScopedTemporaryDirectory base_dir; + ASSERT_TRUE(base_dir.fd().is_valid()); + PersistentCache::SetCacheDirectoryPath(base_dir.path()); + PersistentCache::ResetCacheForProcess(); + // Create 2 dummy SkSL cache file IE (base32 encoding of A), II (base32 // encoding of B) with content x and y. - fml::ScopedTemporaryDirectory temp_dir; - PersistentCache::SetCacheDirectoryPath(temp_dir.path()); - PersistentCache::ResetCacheForProcess(); - std::vector components = {"flutter_engine", - GetFlutterEngineVersion(), "skia", - GetSkiaVersion(), "sksl"}; - auto sksl_dir = fml::CreateDirectory(temp_dir.fd(), components, + std::vector components = { + "flutter_engine", GetFlutterEngineVersion(), "skia", GetSkiaVersion(), + PersistentCache::kSkSLSubdirName}; + auto sksl_dir = fml::CreateDirectory(base_dir.fd(), components, fml::FilePermission::kReadWrite); const std::string x = "x"; const std::string y = "y"; @@ -1270,9 +1283,6 @@ TEST_F(ShellTest, OnServiceProtocolGetSkSLsWorks) { (expected_json2 == buffer.GetString()); ASSERT_TRUE(json_is_expected) << buffer.GetString() << " is not equal to " << expected_json1 << " or " << expected_json2; - - // Cleanup files - fml::RemoveFilesInDirectory(temp_dir.fd()); } TEST_F(ShellTest, RasterizerScreenshot) { diff --git a/engine/src/flutter/testing/fuchsia/run_tests.sh b/engine/src/flutter/testing/fuchsia/run_tests.sh index 4339add946f..16d452cbb97 100755 --- a/engine/src/flutter/testing/fuchsia/run_tests.sh +++ b/engine/src/flutter/testing/fuchsia/run_tests.sh @@ -50,11 +50,11 @@ reboot() { --timeout-seconds $ssh_timeout_seconds \ --identity-file $pkey - echo "$(date) START:REBOOT ------------------------------------------" + echo "$(date) START:REBOOT ----------------------------------------" # note: this will set an exit code of 255, which we can ignore. ./fuchsia_ctl -d $device_name ssh -c "dm reboot-recovery" \ --identity-file $pkey || true - echo "$(date) END:REBOOT --------------------------------------------" + echo "$(date) END:REBOOT ------------------------------------------" } trap reboot EXIT @@ -103,7 +103,7 @@ echo "$(date) DONE:txt_tests ----------------------------------------" # once it passes on Fuchsia. # TODO(https://github.com/flutter/flutter/issues/58211): Re-enable MessageLoop # test once it passes on Fuchsia. -echo "$(date) START:fml_tests ----------------------------------------" +echo "$(date) START:fml_tests ---------------------------------------" ./fuchsia_ctl -d $device_name test \ -f fml_tests-0.far \ -t fml_tests \ @@ -163,14 +163,10 @@ echo "$(date) START:ui_tests ----------------------------------------" --packages-directory packages echo "$(date) DONE:ui_tests -----------------------------------------" -# TODO(https://github.com/flutter/flutter/issues/53399): Re-enable -# OnServiceProtocolGetSkSLsWorks, CanLoadSkSLsFromAsset, and -# CanRemoveOldPersistentCache once they pass on Fuchsia. echo "$(date) START:shell_tests -------------------------------------" ./fuchsia_ctl -d $device_name test \ -f shell_tests-0.far \ -t shell_tests \ - -a "--gtest_filter=-ShellTest.CacheSkSLWorks:ShellTest.SetResourceCacheSize*:ShellTest.OnServiceProtocolGetSkSLsWorks:ShellTest.CanLoadSkSLsFromAsset:ShellTest.CanRemoveOldPersistentCache" \ --identity-file $pkey \ --timeout-seconds $test_timeout_seconds \ --packages-directory packages @@ -178,7 +174,6 @@ echo "$(date) START:shell_tests -------------------------------------" ./fuchsia_ctl -d $device_name test \ -f shell_tests_next-0.far \ -t shell_tests_next \ - -a "--gtest_filter=-ShellTest.CacheSkSLWorks:ShellTest.SetResourceCacheSize*:ShellTest.OnServiceProtocolGetSkSLsWorks:ShellTest.CanLoadSkSLsFromAsset:ShellTest.CanRemoveOldPersistentCache" \ --identity-file $pkey \ --timeout-seconds $test_timeout_seconds \ --packages-directory packages diff --git a/engine/src/flutter/vulkan/vulkan_application.cc b/engine/src/flutter/vulkan/vulkan_application.cc index 97012c49a6f..3a1e4b11320 100644 --- a/engine/src/flutter/vulkan/vulkan_application.cc +++ b/engine/src/flutter/vulkan/vulkan_application.cc @@ -107,15 +107,15 @@ VulkanApplication::VulkanApplication( } instance_ = {instance, [this](VkInstance i) { - FML_LOG(INFO) << "Destroying Vulkan instance"; + FML_DLOG(INFO) << "Destroying Vulkan instance"; vk.DestroyInstance(i, nullptr); }}; if (enable_instance_debugging) { auto debug_report = std::make_unique(vk, instance_); if (!debug_report->IsValid()) { - FML_LOG(INFO) << "Vulkan debugging was enabled but could not be setup " - "for this instance."; + FML_DLOG(INFO) << "Vulkan debugging was enabled but could not be setup " + "for this instance."; } else { debug_report_ = std::move(debug_report); FML_DLOG(INFO) << "Debug reporting is enabled.";