Refactoring and tests for the highly undertested file_util::CreateOrUpdateShortcutLink() method.

base/file_util_unittest.cc

Index: base/file_util_unittest.cc
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index 5a999839e79612fb5b3aca59c9be3149a6ad2e36..1cc97825f4752375e67191a1274ab106d352b3be 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -6,6 +6,7 @@
 
 #if defined(OS_WIN)
 #include <windows.h>
+#include <objbase.h>
 #include <shellapi.h>
 #include <shlobj.h>
 #include <tchar.h>
@@ -21,6 +22,8 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/test/test_file_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -133,18 +136,6 @@ const wchar_t bogus_content[] = L"I'm cannon fodder.";
 const int FILES_AND_DIRECTORIES =
     file_util::FileEnumerator::FILES | file_util::FileEnumerator::DIRECTORIES;
 
-// file_util winds up using autoreleased objects on the Mac, so this needs
-// to be a PlatformTest
-class FileUtilTest : public PlatformTest {
- protected:
-  virtual void SetUp() OVERRIDE {
-    PlatformTest::SetUp();
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  }
-
-  ScopedTempDir temp_dir_;
-};
-
 // Collects all the results from the given file enumerator, and provides an
 // interface to query whether a given file is present.
 class FindResultCollector {
@@ -307,6 +298,20 @@ static const struct dir_case {
 #endif
 };
 
+// file_util winds up using autoreleased objects on the Mac, so this needs
+// to be a PlatformTest
+class FileUtilTest : public PlatformTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    PlatformTest::SetUp();
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    ASSERT_TRUE(temp_dir_2_.CreateUniqueTempDir());
+  }
+
+  ScopedTempDir temp_dir_;
+  ScopedTempDir temp_dir_2_;
+};
+
 TEST_F(FileUtilTest, CountFilesCreatedAfter) {
   FilePath file_name =
       temp_dir_.path().Append(FILE_PATH_LITERAL("f.txt"));
@@ -397,6 +402,176 @@ TEST_F(FileUtilTest, NormalizeFilePathBasic) {
 
 #if defined(OS_WIN)
 
+class FileUtilShortcutTest : public FileUtilTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    FileUtilTest::SetUp();
+
+    EXPECT_EQ(S_OK, CoInitialize(NULL));
+
+    link_file_ = temp_dir_.path().Append(L"My Link.lnk");
+
+    // Shortcut 1's properties
+    {
+      file_contents_ = string16(L"This is a target.");
+      target_file_ = temp_dir_.path().Append(L"Target 1.txt");
+      CreateTextFile(target_file_, file_contents_);
+
+      link_properties_.set_target(target_file_.value());
+      link_properties_.set_working_dir(temp_dir_.path().value());
+      link_properties_.set_arguments(L"--magic --awesome");
+      link_properties_.set_description(L"Chrome is awesome.");
+      link_properties_.set_icon(link_properties_.target, 4);
+      link_properties_.set_app_id(L"Chrome");
+      link_properties_.set_dual_mode(false);
+    }
+
+    // Shortcut 2's properties (all different from properties of shortcut 1).
+    {
+      file_contents_2_ = string16(L"This is another target.");
+      target_file_2_ = temp_dir_.path().Append(L"Target 2.txt");
+      CreateTextFile(target_file_2_, file_contents_2_);
+
+      icon_path_2_ = temp_dir_.path().Append(L"icon.ico");
+      CreateTextFile(icon_path_2_, L"A fake icon");
+
+      link_properties_2_.set_target(target_file_2_.value());
+      link_properties_2_.set_working_dir(temp_dir_2_.path().value());
+      link_properties_2_.set_arguments(L"--super --crazy");
+      link_properties_2_.set_description(L"The best in the west.");
+      link_properties_2_.set_icon(icon_path_2_.value(), 0);
+      link_properties_2_.set_app_id(L"Chrome.UserLevelCrazySuffix");
+      link_properties_2_.set_dual_mode(true);
+    }
+  }
+
+  virtual void TearDown() OVERRIDE {
+    CoUninitialize();
+  }
+
+  // The link file to be created/updated in the shortcut tests below.
+  FilePath link_file_;
+
+  // Properties for the created shortcut.
+  string16 file_contents_;
+  file_util::ShortcutProperties link_properties_;
+
+  // Properties for the updated shortcut.
+  string16 file_contents_2_;
+  file_util::ShortcutProperties link_properties_2_;
+
+ private:
+  // Keep these paths around for the life of this fixture as they contain
+  // strings pointed to by |link_properties_*| above.

[robertshield, 2012/09/06 02:37:34]

nit: "strings pointed to" -> "paths referenced by"

[gab, 2012/09/06 04:20:02]

Oops, thanks for catching this, this is actually no longer necessary as I'm now
passing string16's by reference; I used to pass in the underlying .c_str() (i.e.
wchar_t* parameters) in which case if the FilePath went out of scope that memory
area was up for grabs early while the pointer to |target| was still trusted....
(this is actually why I switched towards string16+bitfield instead of relying on
pointers+NULL; much safer now imo).

+  FilePath target_file_;
+  FilePath target_file_2_;
+  FilePath icon_path_2_;
+};
+
+TEST_F(FileUtilShortcutTest, CreateShortcut) {
+  file_util::ShortcutProperties only_target_properties;
+  only_target_properties.set_target(link_properties_.target);
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), only_target_properties,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  FilePath resolved_name;
+  EXPECT_TRUE(file_util::ResolveShortcut(link_file_, &resolved_name, NULL));
+  const string16 read_contents(ReadTextFile(resolved_name));
+  EXPECT_EQ(file_contents_, read_contents);
+}
+
+TEST_F(FileUtilShortcutTest, CreateShortcutWithOnlySomeProperties) {
+  file_util::ShortcutProperties target_and_args_properties;
+  target_and_args_properties.set_target(link_properties_.target);
+  target_and_args_properties.set_arguments(link_properties_.arguments);
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), target_and_args_properties,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(),
+                                      target_and_args_properties));
+}
+
+TEST_F(FileUtilShortcutTest, CreateShortcutVerifyProperties) {
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(), link_properties_));
+}
+
+TEST_F(FileUtilShortcutTest, UpdateShortcutVerifyProperties) {
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_2_,
+      file_util::SHORTCUT_UPDATE_EXISTING));
+
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(), link_properties_2_));
+}
+
+TEST_F(FileUtilShortcutTest, UpdateShortcutUpdateOnlyTarget) {
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  file_util::ShortcutProperties update_only_target_properties;
+  update_only_target_properties.set_target(link_properties_2_.target);
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), update_only_target_properties,
+      file_util::SHORTCUT_UPDATE_EXISTING));
+
+  file_util::ShortcutProperties expected_properties = link_properties_;
+  expected_properties.set_target(link_properties_2_.target);
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(), expected_properties));
+}
+
+TEST_F(FileUtilShortcutTest, UpdateShortcutMakeDualMode) {
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  file_util::ShortcutProperties make_dual_mode_properties;
+  make_dual_mode_properties.set_dual_mode(true);
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), make_dual_mode_properties,
+      file_util::SHORTCUT_UPDATE_EXISTING));
+
+  file_util::ShortcutProperties expected_properties = link_properties_;
+  expected_properties.set_dual_mode(true);
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(), expected_properties));
+}
+
+TEST_F(FileUtilShortcutTest, UpdateShortcutRemoveDualMode) {
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), link_properties_2_,
+      file_util::SHORTCUT_CREATE_ALWAYS));
+
+  file_util::ShortcutProperties remove_dual_mode_properties;
+  remove_dual_mode_properties.set_dual_mode(false);
+
+  ASSERT_TRUE(file_util::CreateOrUpdateShortcutLink(
+      link_file_.value().c_str(), remove_dual_mode_properties,
+      file_util::SHORTCUT_UPDATE_EXISTING));
+
+  file_util::ShortcutProperties expected_properties = link_properties_2_;
+  expected_properties.set_dual_mode(false);
+  ASSERT_EQ(file_util::VERIFY_SHORTCUT_SUCCESS,
+            file_util::VerifyShortcut(link_file_.value(), expected_properties));
+}
+
 TEST_F(FileUtilTest, NormalizeFilePathReparsePoints) {
   // Build the following directory structure:
   //
@@ -1639,28 +1814,6 @@ TEST_F(FileUtilTest, ResolveShortcutTest) {
   CoUninitialize();
 }
 
-TEST_F(FileUtilTest, CreateShortcutTest) {
-  const wchar_t* file_contents = L"This is another target.";
-  FilePath target_file = temp_dir_.path().Append(L"Target1.txt");
-  CreateTextFile(target_file, file_contents);
-
-  FilePath link_file = temp_dir_.path().Append(L"Link1.lnk");
-
-  CoInitialize(NULL);
-  EXPECT_TRUE(file_util::CreateOrUpdateShortcutLink(
-                  target_file.value().c_str(), link_file.value().c_str(), NULL,
-                  NULL, NULL, NULL, 0, NULL,
-                  file_util::SHORTCUT_CREATE_ALWAYS));
-  FilePath resolved_name;
-  EXPECT_TRUE(file_util::ResolveShortcut(link_file, &resolved_name, NULL));
-  std::wstring read_contents = ReadTextFile(resolved_name);
-  EXPECT_EQ(file_contents, read_contents);
-
-  DeleteFile(target_file.value().c_str());
-  DeleteFile(link_file.value().c_str());
-  CoUninitialize();
-}
-
 TEST_F(FileUtilTest, CopyAndDeleteDirectoryTest) {
   // Create a directory
   FilePath dir_name_from =