Create ScopedTempDir, implement it on POSIX, and use it where appropriate.

util/test/scoped_temp_dir_test_posix.cc

Index: util/test/scoped_temp_dir_test_posix.cc
diff --git a/util/test/scoped_temp_dir_test_posix.cc b/util/test/scoped_temp_dir_test_posix.cc
new file mode 100644
index 0000000000000000000000000000000000000000..10fe79f4664d35a7b8bd559132b5409d9473f1ce
--- /dev/null
+++ b/util/test/scoped_temp_dir_test_posix.cc
@@ -0,0 +1,107 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.

[Mark Mentovai, 2014/12/31 05:51:10]

I see why you named it this way, but I think that it should be
scoped_temp_dir_posix_test.cc, otherwise it’s hard to identify as a test by the
pattern *_test.*.

Alternatively, just call it scoped_temp_dir_test.cc, and abstract the mkdir()
and creat()/close() into something like CreateDirectory() and CreateFile() which
you implement for POSIX, alongside FileExists().

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/test/scoped_temp_dir.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "base/posix/eintr_wrapper.h"
+#include "gtest/gtest.h"
+#include "util/test/errors.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+bool FileExists(const base::FilePath& path) {
+  struct stat st;
+  int rv = stat(path.value().c_str(), &st);

[Mark Mentovai, 2014/12/31 05:51:10]

I think it’s good to be in the habit of calling lstat() unless you’re really
sure you want to follow symbolic links. Here, you’re only concerned with
directory entries, so lstat() is more appropriate.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+  if (rv < 0) {
+    EXPECT_EQ(ENOENT, errno) << "stat expected ENOENT, got " << strerror(errno);

[Mark Mentovai, 2014/12/31 05:51:10]

The message kinda repeats what the code says, except for the addition of the
strerror bit. Maybe just stream ErrnoMessage("stat") to the EXPECT.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+    return false;
+  }
+  return true;
+}
+
+TEST(ScopedTempDir, Empty) {
+  base::FilePath path;
+  {
+    ScopedTempDir dir;
+    path = dir.path();
+    EXPECT_TRUE(FileExists(path));
+  }
+  EXPECT_FALSE(FileExists(path));
+}
+
+TEST(ScopedTempDir, WithTwoFiles) {
+  base::FilePath parent, file1, file2;
+
+  {
+    ScopedTempDir dir;
+    parent = dir.path();
+    ASSERT_TRUE(FileExists(parent));
+
+    file1 = parent.Append("test1");
+    int fd = HANDLE_EINTR(creat(file1.value().c_str(), 0644));

[Mark Mentovai, 2014/12/31 05:51:10]

The ASSERT_GE(fd, 0) << ErrnoMessage("creat") is missing. But wait…

This is actually another reason to abstract this into to CreateFile(): you’ve
got this two-liner creat() and close() in four places now, and I’m talking about
turning it into a three-liner or more (if you EXPECT on close()’s return value
too, or if CreateFile() returns bool instead of void).

It’s good to can properly interpret the return value from creat() and only call
close() if the creat() succeeded. More importantly, it can also log a better
message with ErrnoMessage() if creat() fails. That will be more useful than just
letting FileExists() fail without any indication that creat() didn’t work or why
it didn’t work. You did the logging on the mkdir() below to handle the new
directory, you should do the same for the new files.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+    IGNORE_EINTR(close(fd));
+    ASSERT_TRUE(FileExists(file1));

[Mark Mentovai, 2014/12/31 05:51:10]

This can be EXPECT_TRUE() since it isn’t a precondition for anything that
follows. Same for the check for file2.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+
+    file2 = parent.Append("test 2");

[Mark Mentovai, 2014/12/31 05:51:10]

Spaces! Happy to see that.

You should also test with a dot-file.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+    fd = HANDLE_EINTR(creat(file2.value().c_str(), 0644));
+    IGNORE_EINTR(close(fd));
+    ASSERT_TRUE(FileExists(file2));
+  }
+
+  EXPECT_FALSE(FileExists(file1));
+  EXPECT_FALSE(FileExists(file2));
+  EXPECT_FALSE(FileExists(parent));
+}
+
+TEST(ScopedTempDir, WithRecursiveDirectory) {
+  base::FilePath parent, file1, child_dir, file2;
+
+  {
+    ScopedTempDir dir;
+    parent = dir.path();
+    ASSERT_TRUE(FileExists(parent));
+
+    file1 = parent.Append("first-level file");
+    int fd = HANDLE_EINTR(creat(file1.value().c_str(), 0644));
+    IGNORE_EINTR(close(fd));
+    ASSERT_TRUE(FileExists(file1));

[Mark Mentovai, 2014/12/31 05:51:10]

Same here and on line 96. Line 91 needs to stay an ASSERT.

[Robert Sesek, 2015/01/02 17:34:09]

Done.

+
+    child_dir = parent.Append("subdir");
+    ASSERT_EQ(0, mkdir(child_dir.value().c_str(), 0755))
+        << ErrnoMessage("mkdir");
+    ASSERT_TRUE(FileExists(child_dir));
+
+    file2 = child_dir.Append("second level file");
+    fd = HANDLE_EINTR(creat(file2.value().c_str(), 0644));
+    IGNORE_EINTR(close(fd));
+    ASSERT_TRUE(FileExists(file2));
+  }
+
+  EXPECT_FALSE(FileExists(file1));
+  EXPECT_FALSE(FileExists(file2));
+  EXPECT_FALSE(FileExists(child_dir));
+  EXPECT_FALSE(FileExists(parent));
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad