ambient: rework ambient photo controller unit test

Fixes flakiness in ShouldSaveAndDeleteImageOnDisk
Add brief delay to TestAmbientURLLoaderImpl
Re-order expectations to avoid races

Bug: b/160720485
Change-Id: I8be82e924d592024b972c4f13a8300418a39dc3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300008
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789513}

ash/ambient/ambient_photo_controller.cc

@@ -40,6 +40,8 @@ namespace ash {
 
 namespace {
 
+// TODO(b/161357364): refactor utility functions and constants
+
 // Topic related numbers.
 
 // The number of requests to fetch topics.
@@ -103,7 +105,6 @@ base::TaskTraits GetTaskTraits() {
           base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN};
 }
 
-// TODO: Move to ambient_util.
 void WriteFile(const base::FilePath& path, const std::string& data) {
   if (!base::PathExists(GetRootPath()) &&
       !base::CreateDirectory(GetRootPath())) {
@@ -227,7 +228,7 @@ AmbientPhotoController::AmbientPhotoController()
 AmbientPhotoController::~AmbientPhotoController() = default;
 
 void AmbientPhotoController::StartScreenUpdate() {
-  root_path_ = GetRootPath().Append(FILE_PATH_LITERAL(base::GenerateGUID()));
+  photo_path_ = GetRootPath().Append(FILE_PATH_LITERAL(base::GenerateGUID()));
   task_runner_->PostTaskAndReply(
       FROM_HERE, base::BindOnce(&DeletePathRecursively, GetRootPath()),
       base::BindOnce(&AmbientPhotoController::FetchTopics,
@@ -242,7 +243,8 @@ void AmbientPhotoController::StopScreenUpdate() {
   weak_factory_.InvalidateWeakPtrs();
 
   task_runner_->PostTask(FROM_HERE,
-                         base::BindOnce(&DeletePathRecursively, root_path_));
+                         base::BindOnce(&DeletePathRecursively, photo_path_));
+  photo_path_.clear();
 }
 
 void AmbientPhotoController::OnTopicsChanged() {
@@ -320,7 +322,7 @@ void AmbientPhotoController::TryReadPhotoRawData() {
   const AmbientModeTopic& topic = GetNextTopic();
   const std::string& image_url = topic.portrait_image_url.value_or(topic.url);
 
-  base::FilePath path = root_path_.Append(ToPhotoFileName(image_url));
+  base::FilePath path = photo_path_.Append(ToPhotoFileName(image_url));
   task_runner_->PostTaskAndReplyWithResult(
       FROM_HERE,
       base::BindOnce(
@@ -367,7 +369,7 @@ void AmbientPhotoController::OnPhotoRawDataAvailable(
     return;
   }
 
-  const base::FilePath path = root_path_.Append(ToPhotoFileName(image_url));
+  const base::FilePath path = photo_path_.Append(ToPhotoFileName(image_url));
   task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(

ash/ambient/ambient_photo_controller.h

@@ -164,7 +164,7 @@ class ASH_EXPORT AmbientPhotoController : public AmbientBackendModelObserver {
   ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver>
       ambient_backend_model_observer_{this};
 
-  base::FilePath root_path_;
+  base::FilePath photo_path_;
 
   std::unique_ptr<AmbientURLLoader> url_loader_;
 

ash/ambient/ambient_photo_controller_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/hash/sha1.h"
@@ -104,26 +105,51 @@ TEST_F(AmbientPhotoControllerTest, ShouldSaveAndDeleteImagesOnDisk) {
   base::FilePath home_dir;
   base::PathService::Get(base::DIR_HOME, &home_dir);
 
-  // Start to refresh images.
-  photo_controller()->StartScreenUpdate();
-  base::FilePath root_path =
+  base::FilePath ambient_image_path =
       home_dir.Append(FILE_PATH_LITERAL(kAmbientModeDirectoryName));
-  EXPECT_TRUE(!base::PathExists(root_path) ||
-              base::IsDirectoryEmpty(root_path));
 
+  // Save a file to check if it gets deleted by StartScreenUpdate.
+  auto file_to_delete = ambient_image_path.Append("file_to_delete");
+  base::WriteFile(file_to_delete, "delete_me");
+
+  // Start to refresh images. Kicks off tasks that cleans |ambient_image_path|,
+  // then downloads a test image and writes it to a subdirectory of
+  // |ambient_image_path| in a delayed task.
+  photo_controller()->StartScreenUpdate();
   task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
+
+  EXPECT_TRUE(base::PathExists(ambient_image_path));
+  EXPECT_FALSE(base::PathExists(file_to_delete));
+
+  {
+    // Count files and directories in root_path. There should only be one
+    // subdirectory that was just created to save image files for this ambient
+    // mode session.
+    base::FileEnumerator files(
+        ambient_image_path, /*recursive=*/false,
+        base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
+    int count = 0;
+    for (base::FilePath current = files.Next(); !current.empty();
+         current = files.Next()) {
+      EXPECT_TRUE(files.GetInfo().IsDirectory());
+      count++;
+    }
+
+    EXPECT_EQ(count, 1);
+  }
+
   auto image = photo_controller()->ambient_backend_model()->GetNextImage();
   EXPECT_FALSE(image.isNull());
-  EXPECT_TRUE(base::PathExists(root_path));
-  EXPECT_FALSE(base::IsDirectoryEmpty(root_path));
 
   // Stop to refresh images.
   photo_controller()->StopScreenUpdate();
   task_environment()->FastForwardBy(1.2 * kPhotoRefreshInterval);
+
+  EXPECT_TRUE(base::PathExists(ambient_image_path));
+  EXPECT_TRUE(base::IsDirectoryEmpty(ambient_image_path));
+
   image = photo_controller()->ambient_backend_model()->GetNextImage();
   EXPECT_TRUE(image.isNull());
-  EXPECT_TRUE(base::PathExists(root_path));
-  EXPECT_TRUE(base::IsDirectoryEmpty(root_path));
 }
 
 }  // namespace ash

ash/ambient/test/ambient_ash_test_base.cc

@@ -19,7 +19,9 @@
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/sequenced_task_runner.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/dbus/power/power_manager_client.h"
@@ -41,8 +43,9 @@ class TestAmbientURLLoaderImpl : public AmbientURLLoader {
     auto data = std::make_unique<std::string>();
     *data = "test";
     // Pretend to respond asynchronously.
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), std::move(data)));
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(data)),
+        base::TimeDelta::FromMilliseconds(1));
   }
 };