Fix broken shell_unittests on Fuchsia (flutter#20422)

Fixes flutter/flutter#53399
zanderso · Aug 12, 2020 · 6381b15 · 6381b15
1 parent b7122a1
commit 6381b15
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 32 deletions.
diff --git a/fml/file.cc b/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/shell/common/BUILD.gn b/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/shell/common/shell_unittests.cc b/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<size_t>(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<std::string> components = {"flutter_engine",
-                                         GetFlutterEngineVersion(), "skia",
-                                         GetSkiaVersion(), "sksl"};
-  auto sksl_dir = fml::CreateDirectory(temp_dir.fd(), components,
+  std::vector<std::string> 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/testing/fuchsia/run_tests.sh b/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,22 +163,17 @@ 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
 
 ./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/vulkan/vulkan_application.cc b/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<VulkanDebugReport>(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.";