Commit 29713fc8 authored by Sophie Chang's avatar Sophie Chang Committed by Chromium LUCI CQ

Attempt to fix flake around file deletion in PredictionModelDownloadManager test

This uses the suggested mitigation step around retrying until the last
file error is not ACCESS_DENIED, as suggested in crbug/1156112#c1

Bug: 1156112
Change-Id: I6e269be3136685cabf029d95d291c71ab2fd0b05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622850
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: default avatarMichael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842205}
parent 9e774390
...@@ -154,6 +154,32 @@ class PredictionModelDownloadManagerTest : public testing::Test { ...@@ -154,6 +154,32 @@ class PredictionModelDownloadManagerTest : public testing::Test {
switches::kDisableModelDownloadVerificationForTesting); switches::kDisableModelDownloadVerificationForTesting);
} }
// Retries until the path has been deleted or until all handles to |path| have
// been closed. Returns whether |path| has been deleted.
//
// See crbug/1156112#c1 for suggested mitigation steps.
bool HasPathBeenDeleted(const base::FilePath& path) {
while (true) {
RunUntilIdle();
bool path_exists = base::PathExists(path);
if (!path_exists)
return true;
base::File::Error file_error = base::File::GetLastFileError();
// In the event this does not fix the flake, log the error so we know what
// it is.
// TODO(crbug/1156112): Remove this log once the flake has been resolved.
DLOG(ERROR) << "Path Exists Error: " << file_error;
if (file_error != base::File::FILE_ERROR_ACCESS_DENIED)
return !path_exists;
// Retry if the last file error is access denied since it's likely that
// the file is in the process of being deleted.
}
}
private: private:
void WriteFileForStatus(PredictionModelDownloadFileStatus status) { void WriteFileForStatus(PredictionModelDownloadFileStatus status) {
if (status == PredictionModelDownloadFileStatus::kVerifiedCrxWithNoFiles || if (status == PredictionModelDownloadFileStatus::kVerifiedCrxWithNoFiles ||
...@@ -383,7 +409,7 @@ TEST_F(PredictionModelDownloadManagerTest, UnverifiedFileShouldDeleteTempFile) { ...@@ -383,7 +409,7 @@ TEST_F(PredictionModelDownloadManagerTest, UnverifiedFileShouldDeleteTempFile) {
RunUntilIdle(); RunUntilIdle();
EXPECT_FALSE(observer.last_ready_model().has_value()); EXPECT_FALSE(observer.last_ready_model().has_value());
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus::kUnverifiedFile))); PredictionModelDownloadFileStatus::kUnverifiedFile)));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
"OptimizationGuide.PredictionModelDownloadManager." "OptimizationGuide.PredictionModelDownloadManager."
...@@ -403,7 +429,7 @@ TEST_F(PredictionModelDownloadManagerTest, ...@@ -403,7 +429,7 @@ TEST_F(PredictionModelDownloadManagerTest,
RunUntilIdle(); RunUntilIdle();
EXPECT_FALSE(observer.last_ready_model().has_value()); EXPECT_FALSE(observer.last_ready_model().has_value());
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus::kVerifiedCrxWithNoFiles))); PredictionModelDownloadFileStatus::kVerifiedCrxWithNoFiles)));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
...@@ -426,7 +452,7 @@ TEST_F(PredictionModelDownloadManagerTest, ...@@ -426,7 +452,7 @@ TEST_F(PredictionModelDownloadManagerTest,
RunUntilIdle(); RunUntilIdle();
EXPECT_FALSE(observer.last_ready_model().has_value()); EXPECT_FALSE(observer.last_ready_model().has_value());
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus::kVerifiedCrxWithBadModelInfoFile))); PredictionModelDownloadFileStatus::kVerifiedCrxWithBadModelInfoFile)));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
...@@ -449,7 +475,7 @@ TEST_F(PredictionModelDownloadManagerTest, ...@@ -449,7 +475,7 @@ TEST_F(PredictionModelDownloadManagerTest,
RunUntilIdle(); RunUntilIdle();
EXPECT_FALSE(observer.last_ready_model().has_value()); EXPECT_FALSE(observer.last_ready_model().has_value());
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus::kVerifiedCrxWithInvalidModelInfo))); PredictionModelDownloadFileStatus::kVerifiedCrxWithInvalidModelInfo)));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
...@@ -471,7 +497,7 @@ TEST_F(PredictionModelDownloadManagerTest, ...@@ -471,7 +497,7 @@ TEST_F(PredictionModelDownloadManagerTest,
RunUntilIdle(); RunUntilIdle();
EXPECT_FALSE(observer.last_ready_model().has_value()); EXPECT_FALSE(observer.last_ready_model().has_value());
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus:: PredictionModelDownloadFileStatus::
kVerfiedCrxWithValidModelInfoNoModelFile))); kVerfiedCrxWithValidModelInfoNoModelFile)));
...@@ -506,7 +532,7 @@ TEST_F( ...@@ -506,7 +532,7 @@ TEST_F(
.value(), .value(),
FILE_PATH_LITERAL("OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD_123.tflite")); FILE_PATH_LITERAL("OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD_123.tflite"));
// Downloaded file should still be deleted. // Downloaded file should still be deleted.
EXPECT_FALSE(base::PathExists(GetFilePathForDownloadFileStatus( EXPECT_TRUE(HasPathBeenDeleted(GetFilePathForDownloadFileStatus(
PredictionModelDownloadFileStatus::kVerifiedCrxWithGoodModelFiles))); PredictionModelDownloadFileStatus::kVerifiedCrxWithGoodModelFiles)));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment