implement our own temporary file management during integration tests

Summary:
Unfortunately the Python standard library's `tempfile.NamedTemporaryFile`
class does not work very well on Windows: even though the temporary files are
named, they cannot actually be opened by other processes.  Attempting to open
the file fails with a permission denied error.

This code extends our existing temporary directory management logic to also
add APIs for creating individual temporary files.  This has the advantage of
better grouping all temporary files for a given test in the same directory.  I
also updated the code to include the test function name in the temporary
directory prefix.  This should make it a little easier to identify which test
is at fault when there are temporary files left behind after a test.  (This
can happen if the test gets killed or if it leaves behind files or mount
points that cannot be removed normally).

Reviewed By: wez

Differential Revision: D20885160

fbshipit-source-id: 7267b2352e51214354eab7ead839bf166d690974

eden/integration/lib/gitrepo.py

@@ -7,11 +7,12 @@
 import datetime
 import os
 import subprocess
-import tempfile
 import time
 import typing
 from typing import Dict, List, Optional
 
+from eden.test_support.temporary_directory import TempFileManager
+
 from . import repobase
 from .error import CommandError
 from .find_executables import FindExe
@@ -22,9 +23,13 @@ class GitError(CommandError):
 
 
 class GitRepository(repobase.Repository):
-    def __init__(self, path: str) -> None:
+    git_bin: str
+    temp_mgr: TempFileManager
+
+    def __init__(self, path: str, temp_mgr: Optional[TempFileManager] = None) -> None:
         super().__init__(path)
-        self.git_bin = FindExe.GIT
+        self.git_bin = FindExe.GIT  # pyre-ignore[8]: T38947910
+        self.temp_mgr = temp_mgr or TempFileManager("gitrepo")
 
     def git(
         self, *args: str, encoding: str = "utf-8", env: Optional[Dict[str, str]] = None
@@ -128,9 +133,7 @@ class GitRepository(repobase.Repository):
             "GIT_COMMITTER_DATE": committer_date_str,
         }
 
-        with tempfile.NamedTemporaryFile(
-            prefix="eden_commit_msg.", mode="w", encoding="utf-8"
-        ) as msgf:
+        with self.temp_mgr.make_temp_file("git_commit.") as msgf:
             msgf.write(message)
             msgf.flush()
 

eden/integration/lib/hgrepo.py

@@ -9,11 +9,12 @@ import datetime
 import json
 import os
 import subprocess
-import tempfile
 import textwrap
 import typing
 from typing import Any, Dict, List, Optional
 
+from eden.test_support.temporary_directory import TempFileManager
+
 from . import repobase
 from .error import CommandError
 from .find_executables import FindExe
@@ -24,12 +25,22 @@ class HgError(CommandError):
 
 
 class HgRepository(repobase.Repository):
-    def __init__(self, path: str, system_hgrc: Optional[str] = None) -> None:
+    hg_bin: str
+    hg_environment: Dict[str, str]
+    temp_mgr: TempFileManager
+
+    def __init__(
+        self,
+        path: str,
+        system_hgrc: Optional[str] = None,
+        temp_mgr: Optional[TempFileManager] = None,
+    ) -> None:
         """
         If hgrc is specified, it will be used as the value of the HGRCPATH
         environment variable when `hg` is run.
         """
         super().__init__(path)
+        self.temp_mgr = temp_mgr or TempFileManager("hgrepo")
         self.hg_environment = os.environ.copy()
         # Drop any environment variables starting with 'HG'
         # to ensure the user's environment does not affect the tests.
@@ -37,6 +48,7 @@ class HgRepository(repobase.Repository):
             k: v for k, v in os.environ.items() if not k.startswith("HG")
         }
         self.hg_environment["HGPLAIN"] = "1"
+        # pyre-ignore[6]: T38947910
         self.hg_environment["HG_REAL_BIN"] = FindExe.HG_REAL
         self.hg_environment["NOSCMLOG"] = "1"
         # Set HGRCPATH to make sure we aren't affected by the local system's
@@ -45,7 +57,7 @@ class HgRepository(repobase.Repository):
             self.hg_environment["HGRCPATH"] = system_hgrc
         else:
             self.hg_environment["HGRCPATH"] = ""
-        self.hg_bin = FindExe.HG
+        self.hg_bin = FindExe.HG  # pyre-ignore[8]: T38947910
 
     @classmethod
     def get_system_hgrc_contents(cls) -> str:
@@ -222,9 +234,7 @@ class HgRepository(repobase.Repository):
 
         user_config = "ui.username={} <{}>".format(author_name, author_email)
 
-        with tempfile.NamedTemporaryFile(
-            prefix="eden_commit_msg.", mode="w", encoding="utf-8"
-        ) as msgf:
+        with self.temp_mgr.make_temp_file(prefix="hg_commit_msg.") as msgf:
             msgf.write(message)
             msgf.flush()
 

eden/integration/lib/testcase.py

@@ -28,7 +28,7 @@ from typing import (
 )
 
 from eden.test_support.environment_variable import EnvironmentVariableMixin
-from eden.test_support.temporary_directory import TemporaryDirectoryMixin
+from eden.test_support.temporary_directory import TempFileManager
 from eden.thrift import EdenClient
 
 from . import edenclient, gitrepo, hgrepo, repobase
@@ -42,9 +42,7 @@ else:
 
 
 @unittest.skipIf(not edenclient.can_run_eden(), "unable to run edenfs")
-class EdenTestCase(
-    unittest.TestCase, EnvironmentVariableMixin, TemporaryDirectoryMixin
-):
+class EdenTestCase(unittest.TestCase, EnvironmentVariableMixin):
     """
     Base class for eden integration test cases.
 
@@ -56,6 +54,7 @@ class EdenTestCase(
     eden: edenclient.EdenFS
     start: float
     last_event: float
+    temp_mgr: TempFileManager
 
     # Override enable_fault_injection to True in subclasses to enable Eden's fault
     # injection framework when starting edenfs
@@ -77,26 +76,42 @@ class EdenTestCase(
         logging.info("=== %s at %.03fs (+%0.3fs)", event, since_start, since_last)
         self.last_event = now
 
+    def _get_tmp_prefix(self) -> str:
+        """Get a prefix to use for the test's temporary directory name. """
+        # Attempt to include a potion of the test name in the temporary directory
+        # prefix, but limit it to 20 characters.  If the path is too long EdenFS will
+        # fail to start since its Unix socket path won't fit in sockaddr_un, which has a
+        # 108 byte maximum path length.
+        method_name = self._testMethodName
+        for strip_prefix in ("test_", "test"):
+            if method_name.startswith(strip_prefix):
+                method_name = method_name[len(strip_prefix) :]
+                break
+        return f"eden_test.{method_name[:20]}."
+
     def setUp(self) -> None:
         self.start = time.time()
         self.last_event = self.start
         self.system_hgrc: Optional[str] = None
 
-        # Set an environment variable to prevent telemetry logging
-        # during integration tests
-        os.environ["INTEGRATION_TEST"] = "1"
-
         # Add a cleanup event just to log once the other cleanup
         # actions have completed.
         self.addCleanup(self.report_time, "clean up done")
 
+        self.temp_mgr = TempFileManager(self._get_tmp_prefix())
+        self.addCleanup(self.temp_mgr.cleanup)
+
+        # Set an environment variable to prevent telemetry logging
+        # during integration tests
+        os.environ["INTEGRATION_TEST"] = "1"
+
         self.setup_eden_test()
         self.report_time("test setup done")
 
         self.addCleanup(self.report_time, "clean up started")
 
     def setup_eden_test(self) -> None:
-        self.tmp_dir = self.make_temporary_directory()
+        self.tmp_dir = self.temp_mgr.top_level_tmp_dir()
 
         # Place scratch configuration somewhere deterministic for the tests
         scratch_config_file = os.path.join(self.tmp_dir, "scratch.toml")
@@ -159,6 +174,9 @@ class EdenTestCase(
     def mount_path_bytes(self) -> bytes:
         return bytes(self.mount_path)
 
+    def make_temporary_directory(self, prefix: Optional[str] = None) -> str:
+        return str(self.temp_mgr.make_temp_dir(prefix=prefix))
+
     def get_thrift_client(self) -> EdenClient:
         """
         Get a thrift client to the edenfs daemon.
@@ -202,7 +220,9 @@ class EdenTestCase(
                 f.write(hgrepo.HgRepository.get_system_hgrc_contents())
             self.system_hgrc = system_hgrc_path
 
-        repo = hgrepo.HgRepository(repo_path, system_hgrc=self.system_hgrc)
+        repo = hgrepo.HgRepository(
+            repo_path, system_hgrc=self.system_hgrc, temp_mgr=self.temp_mgr
+        )
         repo.init(hgrc=hgrc)
 
         return repo
@@ -210,7 +230,7 @@ class EdenTestCase(
     def create_git_repo(self, name: str) -> gitrepo.GitRepository:
         repo_path = os.path.join(self.repos_dir, name)
         os.mkdir(repo_path)
-        repo = gitrepo.GitRepository(repo_path)
+        repo = gitrepo.GitRepository(repo_path, temp_mgr=self.temp_mgr)
         repo.init()
 
         return repo

eden/test_support/temporary_directory.py

@@ -4,6 +4,8 @@
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2.
 
+# pyre-strict
+
 import abc
 import logging
 import os
@@ -13,7 +15,18 @@ import tempfile
 import types
 import typing
 from pathlib import Path
-from typing import Any, Callable, Tuple, Type, Union
+from typing import (
+    Any,
+    BinaryIO,
+    Callable,
+    Generic,
+    Optional,
+    TextIO,
+    Tuple,
+    Type,
+    TypeVar,
+    Union,
+)
 
 
 def cleanup_tmp_dir(tmp_dir: Path) -> None:
@@ -29,9 +42,9 @@ def cleanup_tmp_dir(tmp_dir: Path) -> None:
     # If we encounter an EPERM or EACCESS error removing a file try making its parent
     # directory writable and then retry the removal.
     def _remove_readonly(
-        func: Callable[[Union[os.PathLike, str]], Any],
+        func: Callable[[Union[os.PathLike, str]], Any],  # pyre-fixme[2]
         path: Union[os.PathLike, str],
-        exc_info: Tuple[Type, BaseException, types.TracebackType],
+        exc_info: Tuple[Type[BaseException], BaseException, types.TracebackType],
     ) -> None:
         _ex_type, ex, _traceback = exc_info
         # pyre-fixme[29]: `Union[Callable[[object], bool], Callable[[object], bool],
@@ -60,22 +73,152 @@ def cleanup_tmp_dir(tmp_dir: Path) -> None:
     shutil.rmtree(tmp_dir, onerror=_remove_readonly)
 
 
-class TemporaryDirectoryMixin(metaclass=abc.ABCMeta):
-    def make_temporary_directory(self) -> str:
-        def clean_up(path_str: str) -> None:
-            if os.environ.get("EDEN_TEST_NO_CLEANUP"):
-                print("Leaving behind eden test directory %r" % path_str)
-            else:
-                cleanup_tmp_dir(pathlib.Path(path_str))
+class TempFileManager:
+    """TempFileManager exists for managing a set of temporary files and directories.
 
-        path_str = tempfile.mkdtemp(prefix="eden_test.")
-        self.addCleanup(lambda: clean_up(path_str))
-        return path_str
+    It creates all temporary files and directories in a single top-level directory,
+    which can later be cleaned up in one pass.
+
+    This helps make it a little easier to track temporary test artifacts while
+    debugging, and helps make it easier to identify when tests have failed to clean up
+    their temporary files.
+
+    This is also necessary on Windows because the standard tempfile.NamedTemporaryFile
+    class unfortunately does not work well there: the temporary files it creates cannot
+    be opened by other processes.
+    """
+
+    _temp_dir: Optional[Path] = None
+    _prefix: Optional[str]
+
+    def __init__(self, prefix: Optional[str] = "eden_test.") -> None:
+        self._prefix = prefix
+
+    def __enter__(self) -> "TempFileManager":
+        return self
+
+    def __exit__(
+        self,
+        exc_type: Optional[Type[BaseException]],
+        exc_value: Optional[BaseException],
+        tb: Optional[types.TracebackType],
+    ) -> None:
+        self.cleanup()
+
+    def cleanup(self) -> None:
+        temp_dir = self._temp_dir
+        if temp_dir is None:
+            return
+
+        if os.environ.get("EDEN_TEST_NO_CLEANUP"):
+            print(f"Leaving behind eden test directory {temp_dir}")
+        cleanup_tmp_dir(temp_dir)
+        self._temp_dir = None
+
+    def make_temp_dir(self, prefix: Optional[str] = None) -> Path:
+        top_level = self.top_level_tmp_dir()
+        path_str = tempfile.mkdtemp(prefix=prefix, dir=str(top_level))
+        return Path(path_str)
+
+    def make_temp_file(
+        self, prefix: Optional[str] = None, mode: str = "r+"
+    ) -> "TemporaryTextFile":
+        top_level = self.top_level_tmp_dir()
+        fd, path_str = tempfile.mkstemp(prefix=prefix, dir=str(top_level))
+        file_obj = os.fdopen(fd, mode, encoding="utf-8")
+        return TemporaryTextFile(file_obj, Path(path_str))
+
+    def make_temp_binary(
+        self, prefix: Optional[str] = None, mode: str = "rb+"
+    ) -> "TemporaryBinaryFile":
+        top_level = self.top_level_tmp_dir()
+        fd, path_str = tempfile.mkstemp(prefix=prefix, dir=str(top_level))
+        file_obj = os.fdopen(fd, mode)
+        return TemporaryBinaryFile(file_obj, Path(path_str))
+
+    def top_level_tmp_dir(self) -> Path:
+        top = self._temp_dir
+        if top is None:
+            top = Path(tempfile.mkdtemp(prefix=self._prefix))
+            self._temp_dir = top
+
+        return top
+
+    def set_tmp_prefix(self, prefix: str) -> None:
+        if self._temp_dir is not None:
+            logging.warning(
+                f"cannot update temporary directory prefix to {prefix}: "
+                f"temporary directory {self._temp_dir} was already created"
+            )
+            return
+        self._prefix = prefix
+
+
+IOType = TypeVar("IOType", TextIO, BinaryIO)
+T = TypeVar("T")
+
+
+class TemporaryFileBase(Generic[IOType]):
+    """This class is largely equivalent to tempfile.NamedTemporaryFile,
+    but it also works on Windows.  (The standard library NamedTemporaryFile class
+    creates files that cannot be opened by other processes.)
+
+    We don't have any logic for closing the file here since the entire containing
+    directory will eventually be removed by TempFileManager.
+    """
+
+    file: IOType
+    name: str
+    path: Path
+
+    def __init__(self, file: IOType, path: Path) -> None:
+        self.file = file
+        self.path = path
+        self.name = str(path)
+
+    def __getattr__(self, name: str) -> Any:  # pyre-fixme[3]
+        if name in ("name", "path"):
+            return self.__dict__[name]
+        else:
+            file = self.__dict__["file"]
+            value = getattr(file, name)
+            setattr(self, name, value)
+            return value
+
+    def __enter__(self: T) -> T:
+        return self
+
+    def __exit__(
+        self,
+        exc_type: Optional[Type[BaseException]],
+        exc_value: Optional[BaseException],
+        tb: Optional[types.TracebackType],
+    ) -> None:
+        pass
+
+
+class TemporaryTextFile(TemporaryFileBase[TextIO]):
+    pass
+
+
+class TemporaryBinaryFile(TemporaryFileBase[BinaryIO]):
+    pass
+
+
+class TemporaryDirectoryMixin(metaclass=abc.ABCMeta):
+    temp_file_manager: TempFileManager = TempFileManager()
+    _temp_cleanup_added: bool = False
+
+    def make_temporary_directory(self, prefix: Optional[str] = None) -> str:
+        self._ensure_temp_cleanup()
+        return str(self.temp_file_manager.make_temp_dir(prefix=prefix))
+
+    def _ensure_temp_cleanup(self) -> None:
+        if not self._temp_cleanup_added:
+            self.addCleanup(self.temp_file_manager.cleanup)
+            self._temp_cleanup_added = True
 
     def addCleanup(
-        self,
-        function: typing.Callable[..., typing.Any],
-        *args: typing.Any,
-        **kwargs: typing.Any,
+        self, function: Callable[[], None], *args: Any, **kwargs: Any
     ) -> None:
         raise NotImplementedError()