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

base/file_util_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
+#include <objbase.h>
 #include <shellapi.h>
 #include <shlobj.h>
 #include <tchar.h>
 #include <winioctl.h>
 #endif
 
 #include <algorithm>
 #include <fstream>
 #include <set>
 
 #include "base/base_paths.h"
 #include "base/file_path.h"
 #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"
 #include "testing/platform_test.h"
 
 #if defined(OS_WIN)
 #include "base/win/scoped_handle.h"
 #endif
 
 // This macro helps avoid wrapped lines in the test structs.
@@ skipping 92 matching lines @@
   mode &= ~mode_bits_to_clear;
   ASSERT_TRUE(file_util::SetPosixFilePermissions(path, mode));
 }
 #endif  // defined(OS_POSIX)
 
 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 {
  public:
   explicit FindResultCollector(file_util::FileEnumerator& enumerator) {
     FilePath cur_file;
     while (!(cur_file = enumerator.Next()).value().empty()) {
       FilePath::StringType path = cur_file.value();
       // The file should not be returned twice.
       EXPECT_TRUE(files_.end() == files_.find(path))
@@ skipping 142 matching lines @@
   {L"/foo/bar/", L"/foo/bar"},
   {L"/foo/bar//", L"/foo/bar"},
   {L"/foo/bar", L"/foo"},
   {L"/foo/bar./", L"/foo/bar."},
   {L"/", L"/"},
   {L".", L"."},
   {L"..", L"."},  // yes, ".." technically lives in "."
 #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"));
   CreateTextFile(file_name, L"test");
 
   base::PlatformFileInfo info;
   file_util::GetFileInfo(file_name, &info);
   base::Time file_time = info.creation_time;
 
   base::TimeDelta two_secs = base::TimeDelta::FromSeconds(2);
@@ skipping 70 matching lines @@
 
   // Beacuse this test created |dir_path|, we know it is not a link
   // or junction.  So, the real path of the directory holding file a
   // must be the parent of the path holding file b.
   ASSERT_TRUE(normalized_file_a_path.DirName()
       .IsParent(normalized_file_b_path.DirName()));
 }
 
 #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:
   //
   // temp_dir
   // |-> base_a
   // |   |-> sub_a
   // |       |-> file.txt
   // |       |-> long_name___... (Very long name.)
   // |           |-> sub_long
   // |              |-> deep.txt
@@ skipping 1222 matching lines @@
   std::wstring contents;
   contents = ReadTextFile(link_file);
   EXPECT_EQ(L"This is the target.", contents);
 
   // Cleaning
   DeleteFile(target_file.value().c_str());
   DeleteFile(link_file.value().c_str());
   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 =
       temp_dir_.path().Append(FILE_PATH_LITERAL("CopyAndDelete_From_Subdir"));
   file_util::CreateDirectory(dir_name_from);
   ASSERT_TRUE(file_util::PathExists(dir_name_from));
 
   // Create a file under the directory
   FilePath file_name_from =
       dir_name_from.Append(FILE_PATH_LITERAL("CopyAndDelete_Test_File.txt"));
@@ skipping 784 matching lines @@
       file_util::VerifyPathControlledByUser(
           base_dir_, text_file_, uid_, ok_gids_));
   EXPECT_TRUE(
       file_util::VerifyPathControlledByUser(
           sub_dir_, text_file_, uid_, ok_gids_));
 }
 
 #endif  // defined(OS_POSIX)
 
 }  // namespace