Replacing base::DIR_TEMP with ScopedTempDir when appropriate.

chrome/browser/profiles/profile_manager_unittest.cc

 // Copyright (c) 2011 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 <string>
 
 #include "base/command_line.h"
-#include "base/file_util.h"
+#include "base/memory/scoped_temp_dir.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/testing_browser_process.h"
 #include "chrome/test/testing_pref_service.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/notification_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/system_monitor/system_monitor.h"
 
 class ProfileManagerTest : public testing::Test {
  protected:
   ProfileManagerTest()
       : ui_thread_(BrowserThread::UI, &message_loop_),
         file_thread_(BrowserThread::FILE, &message_loop_) {
   }
 
   virtual void SetUp() {
-    // Name a subdirectory of the temp directory.
-    ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &test_dir_));
-    test_dir_ = test_dir_.Append(FILE_PATH_LITERAL("ProfileManagerTest"));
-
-    // Create a fresh, empty copy of this directory.
-    file_util::Delete(test_dir_, true);
-    file_util::CreateDirectory(test_dir_);
+    // Create a new temporary directory, and store the path
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    test_dir_ = temp_dir_.path();

[Paweł Hajdan Jr., 2011/04/04 08:08:54]

How about removing the member variable and using temp_dir_.path() directly in
tests?

[Mike West, 2011/04/04 08:39:33]

Two reasons:

1. This was the minimal changeset, and I'd rather change the least amount code
possible for targeted cleanups (especially since I'm not familiar with most of
the tests I'll be fiddling with :) ).

2. Pulling this out into a temp variable made some sort of aesthetic sense to
me.

That said, I'm happy to change the implementation here if there's a good reason
to do so. Should be a trivial search/replace.

[Paweł Hajdan Jr., 2011/04/04 09:05:31]

Makes sense, but on the other hand it may be worth it to make the cleanup more
complete.
I'm not sure how that could be beneficial. I see is as a duplication of
information, and a potential for those two variables to go out of sync (someone
might have an idea to change test_dir_, and so on).
I think the right thing to do is to make the change.

[Mike West, 2011/04/04 09:16:07]

Fair enough, you've convinced me. :) I'll change it here, and drop it in the
other tests I'll touch later on.

 
     // Create a local_state PrefService.
     browser::RegisterLocalState(&test_local_state_);
     TestingBrowserProcess* testing_browser_process =
         static_cast<TestingBrowserProcess*>(g_browser_process);
     testing_browser_process->SetPrefService(&test_local_state_);
   }
 
   virtual void TearDown() {
     // Clean up test directory
-    ASSERT_TRUE(file_util::Delete(test_dir_, true));
-    ASSERT_FALSE(file_util::PathExists(test_dir_));
+    ASSERT_TRUE(temp_dir_.Delete());

[Paweł Hajdan Jr., 2011/04/04 08:08:54]

This shouldn't be necessary. That's the entire point of the scoper.

[Mike West, 2011/04/04 08:39:33]

Maybe I'm misunderstanding the mechanics here, but I don't believe the object
actually goes out of scope between tests if it's stored as a member variable. If
I don't call `Delete` explicitly in `TearDown`, my expectation is that
`CreateUniqueTempDir` would throw an error in `SetUp` (based on my reading of
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/scoped_t...
), as the variable still exists across tests.

Does the whole `ProfileManagerTest` object get thrown away and recreated for
each test?

[Paweł Hajdan Jr., 2011/04/04 09:05:31]

Yes, it does. Feel free to check with a debugger and/or read GTest docs.
Unfortunately many people are confused by this behavior.

[Mike West, 2011/04/04 09:16:07]

Me, for instance. :)

 
     TestingBrowserProcess* testing_browser_process =
         static_cast<TestingBrowserProcess*>(g_browser_process);
     testing_browser_process->SetPrefService(NULL);
   }
 
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
 
   // the path to temporary directory used to contain the test operations
+  ScopedTempDir temp_dir_;

[Paweł Hajdan Jr., 2011/04/04 08:08:54]

Please make ScopedTempDir the first member variable declared here (before
message_loop_). Some tasks posted to it might want to use the directory.

[Mike West, 2011/04/04 08:39:33]

Done.

   FilePath test_dir_;
 
   TestingPrefService test_local_state_;
 };
 
 TEST_F(ProfileManagerTest, CreateProfile) {
   FilePath source_path;
   PathService::Get(chrome::DIR_TEST_DATA, &source_path);
   source_path = source_path.Append(FILE_PATH_LITERAL("profiles"));
   source_path = source_path.Append(FILE_PATH_LITERAL("sample"));
@@ skipping 90 matching lines @@
 
   // Make sure any pending tasks run before we destroy the profiles.
   message_loop_.RunAllPending();
 
   profile1.reset();
   profile2.reset();
 
   // Make sure history cleans up correctly.
   message_loop_.RunAllPending();
 }