diff --git a/eden/integration/lib/gitrepo.py b/eden/integration/lib/gitrepo.py index 30b2fe50b9..63845b3017 100644 --- a/eden/integration/lib/gitrepo.py +++ b/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() diff --git a/eden/integration/lib/hgrepo.py b/eden/integration/lib/hgrepo.py index 2226e3c520..3c1608db0f 100644 --- a/eden/integration/lib/hgrepo.py +++ b/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() diff --git a/eden/integration/lib/testcase.py b/eden/integration/lib/testcase.py index aa2843638e..31a37bb2a9 100644 --- a/eden/integration/lib/testcase.py +++ b/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 diff --git a/eden/test_support/temporary_directory.py b/eden/test_support/temporary_directory.py index b971eed22e..7041d4c23f 100644 --- a/eden/test_support/temporary_directory.py +++ b/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()