A robust base::DeleteFile.

base/files/file_util_unittest.cc

Index: base/files/file_util_unittest.cc
diff --git a/base/files/file_util_unittest.cc b/base/files/file_util_unittest.cc
index 7ef60a4b29881d0016adf5564956f5d1c749bd0a..a39bc782715da8647eab8cc920513f56a202abbf 100644
--- a/base/files/file_util_unittest.cc
+++ b/base/files/file_util_unittest.cc
@@ -192,6 +192,46 @@ class FileUtilTest : public PlatformTest {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   }
 
+  // Sets the file to read-only.
+  static void SetReadOnly(const FilePath& path, bool read_only) {
+#if defined(OS_WIN)
+    // On Windows, it involves setting/removing the 'readonly' bit.
+    DWORD attrs = GetFileAttributes(path.value().c_str());
+    ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs);

[Nico, 2017/01/10 18:31:55]

I thought ASSERT doesn't do the right thing in helper functions (it returns from
the current frame but the calling frame will just continue)

[grt (UTC plus 2), 2017/01/12 15:07:03]

Indeed! I've added ASSERT_NO_FATAL_FAILURE where relevant. Thanks.

+    ASSERT_TRUE(SetFileAttributes(
+        path.value().c_str(), read_only ? (attrs | FILE_ATTRIBUTE_READONLY)
+                                        : (attrs & ~FILE_ATTRIBUTE_READONLY)));
+
+    DWORD expected =
+        read_only
+            ? ((attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY)) |
+               FILE_ATTRIBUTE_READONLY)
+            : (attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY));
+
+    // Ignore FILE_ATTRIBUTE_NOT_CONTENT_INDEXED if present.
+    attrs = GetFileAttributes(path.value().c_str()) &
+            ~FILE_ATTRIBUTE_NOT_CONTENT_INDEXED;
+    ASSERT_EQ(expected, attrs);
+#else
+    // On all other platforms, it involves removing/setting the write bit.
+    mode_t mode = read_only ? S_IRUSR : (S_IRUSR | S_IWUSR);
+    EXPECT_TRUE(SetPosixFilePermissions(
+        path, DirectoryExists(path) ? (mode | S_IXUSR) : mode));
+#endif
+  }
+
+  static bool IsReadOnly(const FilePath& path) {
+#if defined(OS_WIN)
+    DWORD attrs = GetFileAttributes(path.value().c_str());
+    EXPECT_NE(INVALID_FILE_ATTRIBUTES, attrs);
+    return attrs & FILE_ATTRIBUTE_READONLY;
+#else
+    int mode = 0;
+    EXPECT_TRUE(GetPosixFilePermissions(path, &mode));
+    return !(mode & S_IWUSR);
+#endif
+  }
+
   ScopedTempDir temp_dir_;
 };
 
@@ -667,6 +707,30 @@ TEST_F(FileUtilTest, DeleteFile) {
   EXPECT_FALSE(PathExists(file_name));
 }
 
+TEST_F(FileUtilTest, DeleteReadOnlyFile) {
+  // Create a read-only file

[Nico, 2017/01/10 18:31:55]

nit: style guide says comments should be sentences ending with punctuation (also
elsewhere)

[grt (UTC plus 2), 2017/01/12 15:07:03]

Done.

+  FilePath file_name = temp_dir_.GetPath().Append(FPL("Test DeleteFile 1.txt"));
+  CreateTextFile(file_name, bogus_content);
+  ASSERT_TRUE(PathExists(file_name));
+  SetReadOnly(file_name, true);
+  ASSERT_TRUE(IsReadOnly(file_name));
+
+  // Make sure it's deleted
+  EXPECT_TRUE(DeleteFile(file_name, false));
+  EXPECT_FALSE(PathExists(file_name));
+
+  // Test recursive case, create a new file
+  file_name = temp_dir_.GetPath().Append(FPL("Test DeleteFile 2.txt"));
+  CreateTextFile(file_name, bogus_content);
+  ASSERT_TRUE(PathExists(file_name));
+  SetReadOnly(file_name, true);
+  ASSERT_TRUE(IsReadOnly(file_name));
+
+  // Make sure it's deleted
+  EXPECT_TRUE(DeleteFile(file_name, true));
+  EXPECT_FALSE(PathExists(file_name));
+}
+
 #if defined(OS_POSIX)
 TEST_F(FileUtilTest, DeleteSymlinkToExistentFile) {
   // Create a file.
@@ -882,58 +946,6 @@ TEST_F(FileUtilTest, ExecutableExistsInPath) {
 
 #endif  // defined(OS_POSIX)
 
-#if defined(OS_WIN)
-// Tests that the Delete function works for wild cards, especially
-// with the recursion flag.  Also coincidentally tests PathExists.
-// TODO(erikkay): see if anyone's actually using this feature of the API
-TEST_F(FileUtilTest, DeleteWildCard) {
-  // Create a file and a directory
-  FilePath file_name =
-      temp_dir_.GetPath().Append(FPL("Test DeleteWildCard.txt"));
-  CreateTextFile(file_name, bogus_content);
-  ASSERT_TRUE(PathExists(file_name));
-
-  FilePath subdir_path = temp_dir_.GetPath().Append(FPL("DeleteWildCardDir"));
-  CreateDirectory(subdir_path);
-  ASSERT_TRUE(PathExists(subdir_path));
-
-  // Create the wildcard path
-  FilePath directory_contents = temp_dir_.GetPath();
-  directory_contents = directory_contents.Append(FPL("*"));
-
-  // Delete non-recursively and check that only the file is deleted
-  EXPECT_TRUE(DeleteFile(directory_contents, false));
-  EXPECT_FALSE(PathExists(file_name));
-  EXPECT_TRUE(PathExists(subdir_path));
-
-  // Delete recursively and make sure all contents are deleted
-  EXPECT_TRUE(DeleteFile(directory_contents, true));
-  EXPECT_FALSE(PathExists(file_name));
-  EXPECT_FALSE(PathExists(subdir_path));
-}
-
-// TODO(erikkay): see if anyone's actually using this feature of the API
-TEST_F(FileUtilTest, DeleteNonExistantWildCard) {
-  // Create a file and a directory
-  FilePath subdir_path =
-      temp_dir_.GetPath().Append(FPL("DeleteNonExistantWildCard"));
-  CreateDirectory(subdir_path);
-  ASSERT_TRUE(PathExists(subdir_path));
-
-  // Create the wildcard path
-  FilePath directory_contents = subdir_path;
-  directory_contents = directory_contents.Append(FPL("*"));
-
-  // Delete non-recursively and check nothing got deleted
-  EXPECT_TRUE(DeleteFile(directory_contents, false));
-  EXPECT_TRUE(PathExists(subdir_path));
-
-  // Delete recursively and check nothing got deleted
-  EXPECT_TRUE(DeleteFile(directory_contents, true));
-  EXPECT_TRUE(PathExists(subdir_path));
-}
-#endif
-
 // Tests non-recursive Delete() for a directory.
 TEST_F(FileUtilTest, DeleteDirNonRecursive) {
   // Create a subdirectory and put a file and two directories inside.
@@ -1425,46 +1437,6 @@ TEST_F(FileUtilTest, CopyDirectoryWithTrailingSeparators) {
   EXPECT_TRUE(PathExists(file_name_to));
 }
 
-// Sets the source file to read-only.
-void SetReadOnly(const FilePath& path, bool read_only) {
-#if defined(OS_WIN)
-  // On Windows, it involves setting/removing the 'readonly' bit.
-  DWORD attrs = GetFileAttributes(path.value().c_str());
-  ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs);
-  ASSERT_TRUE(SetFileAttributes(
-      path.value().c_str(),
-      read_only ? (attrs | FILE_ATTRIBUTE_READONLY) :
-          (attrs & ~FILE_ATTRIBUTE_READONLY)));
-
-  DWORD expected = read_only ?
-      ((attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY)) |
-          FILE_ATTRIBUTE_READONLY) :
-      (attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY));
-
-  // Ignore FILE_ATTRIBUTE_NOT_CONTENT_INDEXED if present.
-  attrs = GetFileAttributes(path.value().c_str()) &
-          ~FILE_ATTRIBUTE_NOT_CONTENT_INDEXED;
-  ASSERT_EQ(expected, attrs);
-#else
-  // On all other platforms, it involves removing/setting the write bit.
-  mode_t mode = read_only ? S_IRUSR : (S_IRUSR | S_IWUSR);
-  EXPECT_TRUE(SetPosixFilePermissions(
-      path, DirectoryExists(path) ? (mode | S_IXUSR) : mode));
-#endif
-}
-
-bool IsReadOnly(const FilePath& path) {
-#if defined(OS_WIN)
-  DWORD attrs = GetFileAttributes(path.value().c_str());
-  EXPECT_NE(INVALID_FILE_ATTRIBUTES, attrs);
-  return attrs & FILE_ATTRIBUTE_READONLY;
-#else
-  int mode = 0;
-  EXPECT_TRUE(GetPosixFilePermissions(path, &mode));
-  return !(mode & S_IWUSR);
-#endif
-}
-
 TEST_F(FileUtilTest, CopyDirectoryACL) {
   // Create source directories.
   FilePath src = temp_dir_.GetPath().Append(FILE_PATH_LITERAL("src"));