diff --git a/eden/integration/clone_test.py b/eden/integration/clone_test.py index ee549c35e3..52277204c5 100644 --- a/eden/integration/clone_test.py +++ b/eden/integration/clone_test.py @@ -12,13 +12,11 @@ from pathlib import Path from textwrap import dedent from typing import Optional, Sequence, Set -import pexpect from eden.integration.lib.hgrepo import HgRepository from .lib import edenclient, testcase from .lib.fake_edenfs import get_fake_edenfs_argv from .lib.find_executables import FindExe -from .lib.pexpect import PexpectAssertionMixin, wait_for_pexpect_process from .lib.service_test_case import ServiceTestCaseBase, SystemdServiceTest, service_test @@ -259,6 +257,7 @@ class CloneTest(testcase.EdenRepoTest): "--daemon-args", *extra_daemon_args, ) + self.exit_stack.callback(self.eden.run_cmd, "stop") self.assertIn("Starting edenfs", clone_output) self.assertTrue(self.eden.is_healthy(), msg="clone should start Eden.") mount_points = {self.mount: "RUNNING", str(tmp): "RUNNING"} @@ -291,7 +290,7 @@ class CloneTest(testcase.EdenRepoTest): self.assertEqual(custom_readme_text, readme_path.read_text()) -class CloneFakeEdenFSTestBase(ServiceTestCaseBase, PexpectAssertionMixin): +class CloneFakeEdenFSTestBase(ServiceTestCaseBase): def setUp(self) -> None: super().setUp() self.eden_dir = Path(self.make_temporary_directory()) @@ -308,28 +307,45 @@ class CloneFakeEdenFSTestBase(ServiceTestCaseBase, PexpectAssertionMixin): repo_path: Path, mount_path: Path, extra_args: Optional[Sequence[str]] = None, - ) -> "pexpect.spawn[str]": - args = ( - ["--config-dir", str(self.eden_dir)] - + self.get_required_eden_cli_args() - + [ # pyre-ignore[6]: T38947910 - "clone", - "--daemon-binary", - FindExe.FAKE_EDENFS, - str(repo_path), - str(mount_path), - ] - ) + ) -> subprocess.CompletedProcess: + eden_cli: str = FindExe.EDEN_CLI # pyre-ignore[9]: T38947910 + fake_edenfs: str = FindExe.FAKE_EDENFS # pyre-ignore[9]: T38947910 + base_args = [ + eden_cli, + "--config-dir", + str(self.eden_dir), + ] + self.get_required_eden_cli_args() + clone_cmd = base_args + [ + "clone", + "--daemon-binary", + fake_edenfs, + str(repo_path), + str(mount_path), + ] if extra_args: - args.extend(extra_args) - return pexpect.spawn( - # pyre-ignore[6]: T38947910 - FindExe.EDEN_CLI, - args, + clone_cmd.extend(extra_args) + + proc = subprocess.run( + clone_cmd, encoding="utf-8", - logfile=sys.stderr, + errors="replace", + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) + stop_cmd = base_args + ["stop"] + self.exit_stack.callback(subprocess.call, stop_cmd) + + # Pass through any output from the clone command + sys.stdout.write(proc.stdout) + sys.stderr.write(proc.stderr) + + # Note that the clone operation will actually fail here, since we started + # fake_edenfs rather than the real edenfs daemon, and it can't mount checkouts. + # However we only care about testing that the daemon got started, so that's + # fine. + return proc + @service_test class CloneFakeEdenFSTest(CloneFakeEdenFSTestBase): @@ -338,15 +354,14 @@ class CloneFakeEdenFSTest(CloneFakeEdenFSTestBase): mount_path = Path(self.make_temporary_directory()) extra_daemon_args = ["--allowExtraArgs", "hello world"] - clone_process = self.spawn_clone( + self.spawn_clone( repo_path=Path(repo.path), mount_path=mount_path, extra_args=["--daemon-args"] + extra_daemon_args, ) - wait_for_pexpect_process(clone_process) argv = get_fake_edenfs_argv(self.eden_dir) - self.assertEquals( + self.assertEqual( argv[-len(extra_daemon_args) :], extra_daemon_args, f"fake_edenfs should have received arguments verbatim\nargv: {argv}", @@ -360,8 +375,9 @@ class CloneFakeEdenFSWithSystemdTest(SystemdServiceTest, CloneFakeEdenFSTestBase clone_process = self.spawn_clone( repo_path=Path(repo.path), mount_path=mount_path ) - clone_process.expect_exact( - "edenfs daemon is not currently running. Starting edenfs..." + self.assertIn( + "edenfs daemon is not currently running. Starting edenfs...", + clone_process.stdout, ) - clone_process.expect_exact("Started edenfs") + self.assertIn("Started edenfs", clone_process.stderr) self.assert_systemd_service_is_active(eden_dir=self.eden_dir) diff --git a/eden/integration/service_log_test.py b/eden/integration/service_log_test.py index 76b1dbaa31..2c71f602a9 100644 --- a/eden/integration/service_log_test.py +++ b/eden/integration/service_log_test.py @@ -56,15 +56,15 @@ class ServiceLogRealEdenFSTest(ManagedFakeEdenFSMixin, ServiceLogTestBase): self.log_file_path.exists(), f"{self.log_file_path} should not exist before starting edenfs", ) - run_eden_start_with_real_daemon( - eden_dir=self.eden_dir, - etc_eden_dir=self.etc_eden_dir, - home_dir=self.home_dir, - systemd=False, - ) - with FakeEdenFS.from_existing_process(eden_dir=self.eden_dir): - self.assertTrue( - self.log_file_path.exists(), - f"edenfs should create {self.log_file_path}", + self.exit_stack.enter_context( + run_eden_start_with_real_daemon( + eden_dir=self.eden_dir, + etc_eden_dir=self.etc_eden_dir, + home_dir=self.home_dir, + systemd=False, ) - self.assertIn("Starting edenfs", self.log_file_path.read_text()) + ) + self.assertTrue( + self.log_file_path.exists(), f"edenfs should create {self.log_file_path}" + ) + self.assertIn("Starting edenfs", self.log_file_path.read_text()) diff --git a/eden/integration/start_test.py b/eden/integration/start_test.py index f6bdb2ed24..4192c3bf16 100644 --- a/eden/integration/start_test.py +++ b/eden/integration/start_test.py @@ -4,14 +4,14 @@ # This software may be used and distributed according to the terms of the # GNU General Public License version 2. +import contextlib import os import pathlib import subprocess import sys import unittest -from typing import List, Optional, Sequence +from typing import Generator, List, Optional, Sequence -import pexpect from eden.fs.cli.config import EdenInstance from eden.fs.cli.util import HealthStatus from fb303_core.ttypes import fb303_status @@ -20,7 +20,6 @@ from .lib import testcase from .lib.edenfs_systemd import EdenFSSystemdMixin from .lib.fake_edenfs import get_fake_edenfs_argv from .lib.find_executables import FindExe -from .lib.pexpect import PexpectAssertionMixin, wait_for_pexpect_process from .lib.service_test_case import ServiceTestCaseBase, SystemdServiceTest, service_test @@ -94,6 +93,11 @@ class StartTest(testcase.EdenTestCase): self.eden.run_cmd("start", "--if-not-running", "--", "--allowRoot") self.assertTrue(self.eden.is_healthy()) + # Stop edenfs. We didn't start it through self.eden.start() + # so the self.eden class doesn't really know it is running and that + # it needs to be shut down. + self.eden.run_cmd("stop") + @testcase.eden_repo_test class StartWithRepoTest(testcase.EdenRepoTest, EdenFSSystemdMixin): @@ -114,11 +118,13 @@ class StartWithRepoTest(testcase.EdenRepoTest, EdenFSSystemdMixin): self.assert_checkout_is_mounted() def run_eden_start(self, systemd: bool) -> None: - run_eden_start_with_real_daemon( - eden_dir=pathlib.Path(self.eden_dir), - etc_eden_dir=pathlib.Path(self.etc_eden_dir), - home_dir=pathlib.Path(self.home_dir), - systemd=systemd, + self.exit_stack.enter_context( + run_eden_start_with_real_daemon( + eden_dir=pathlib.Path(self.eden_dir), + etc_eden_dir=pathlib.Path(self.etc_eden_dir), + home_dir=pathlib.Path(self.home_dir), + systemd=systemd, + ) ) def assert_checkout_is_mounted(self) -> None: @@ -160,50 +166,81 @@ Did you mean to run "eden" instead of "edenfs"? self.assertMultiLineEqual(err, out.stderr.decode("utf-8", errors="replace")) -class StartFakeEdenFSTestBase(ServiceTestCaseBase, PexpectAssertionMixin): +class StartFakeEdenFSTestBase(ServiceTestCaseBase): def setUp(self) -> None: super().setUp() self.eden_dir = self.make_temp_dir("eden") - def spawn_start( + def _get_base_eden_args(self, eden_dir: Optional[pathlib.Path] = None) -> List[str]: + if eden_dir is None: + eden_dir = self.eden_dir + return [ + FindExe.EDEN_CLI, + "--config-dir", + str(eden_dir), + ] + self.get_required_eden_cli_args() + + def expect_start_failure( + self, + msg: str, + eden_dir: Optional[pathlib.Path] = None, + extra_args: Optional[Sequence[str]] = None, + ) -> subprocess.CompletedProcess: + start_cmd = self._get_base_eden_args(eden_dir) + [ # pyre-ignore[6]: T38947910 + "start", + "--daemon-binary", + FindExe.FAKE_EDENFS, + ] + if extra_args: + start_cmd.extend(extra_args) + proc = subprocess.run( + start_cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + encoding="utf-8", + errors="replace", + ) + + # Pass through the command output, to make the test easier to debug + sys.stdout.write(proc.stdout) + sys.stderr.write(proc.stderr) + + self.assertNotEqual(proc.returncode, 0) + self.assertIn(msg, proc.stderr) + return proc + + def start_edenfs( self, eden_dir: Optional[pathlib.Path] = None, extra_args: Optional[Sequence[str]] = None, - ) -> "pexpect.spawn[str]": - if eden_dir is None: - eden_dir = self.eden_dir - args = ( - ["--config-dir", str(eden_dir)] - + self.get_required_eden_cli_args() - + [ # pyre-ignore[6]: T38947910 - "start", - "--daemon-binary", - FindExe.FAKE_EDENFS, - ] - ) + ) -> None: + base_args = self._get_base_eden_args(eden_dir) + start_cmd = base_args + [ # pyre-ignore[6]: T38947910 + "start", + "--daemon-binary", + FindExe.FAKE_EDENFS, + ] if extra_args: - args.extend(extra_args) - return pexpect.spawn( - # pyre-ignore[6]: T38947910 - FindExe.EDEN_CLI, - args, - encoding="utf-8", - logfile=sys.stderr, - ) + start_cmd.extend(extra_args) + subprocess.check_call(start_cmd) + + def cleanup(): + stop_cmd = base_args + ["stop"] + subprocess.call(stop_cmd) + + self.exit_stack.callback(cleanup) @service_test -class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): +class StartFakeEdenFSTest(StartFakeEdenFSTestBase): def test_eden_start_launches_separate_processes_for_separate_eden_dirs( self ) -> None: eden_dir_1 = self.eden_dir eden_dir_2 = self.make_temp_dir("eden2") - start_1_process = self.spawn_start(eden_dir=eden_dir_1) - self.assert_process_succeeds(start_1_process) - start_2_process = self.spawn_start(eden_dir=eden_dir_2) - self.assert_process_succeeds(start_2_process) + self.start_edenfs(eden_dir=eden_dir_1) + self.start_edenfs(eden_dir=eden_dir_2) instance_1_health: HealthStatus = EdenInstance( str(eden_dir_1), etc_eden_dir=None, home_dir=None @@ -233,8 +270,7 @@ class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): def test_daemon_command_arguments_should_forward_to_edenfs(self) -> None: extra_daemon_args = ["--allowExtraArgs", "--", "hello world", "--ignoredOption"] - start_process = self.spawn_start(extra_args=["--"] + extra_daemon_args) - wait_for_pexpect_process(start_process) + self.start_edenfs(extra_args=["--"] + extra_daemon_args) argv = get_fake_edenfs_argv(self.eden_dir) self.assertEqual( @@ -246,7 +282,7 @@ class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): def test_daemon_command_arguments_should_forward_to_edenfs_without_leading_dashdash( self ) -> None: - start_process = self.spawn_start( + self.start_edenfs( extra_args=[ "hello world", "another fake_edenfs argument", @@ -255,7 +291,6 @@ class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): "arg_after_dashdash", ] ) - self.assert_process_succeeds(start_process) expected_extra_daemon_args = [ "hello world", @@ -307,26 +342,17 @@ class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): config_dir_args = ["--config-dir", str(input_path)] else: config_dir_args = [] - args = ( - self.get_required_eden_cli_args() - + config_dir_args - + [ # pyre-ignore[6]: T38947910 - "start", - "--daemon-binary", - FindExe.FAKE_EDENFS, - ] - ) - start_process: pexpect.spawn[str] = pexpect.spawn( - # pyre-ignore[6]: T38947910 - FindExe.EDEN_CLI, - args, - encoding="utf-8", - logfile=sys.stderr, - ) - wait_for_pexpect_process(start_process) - - argv = get_fake_edenfs_argv(resolved_path) - self.assert_eden_dir(argv, resolved_path) + eden_cli: str = FindExe.EDEN_CLI # pyre-ignore[9]: T38947910 + fake_edenfs: str = FindExe.FAKE_EDENFS # pyre-ignore[9]: T38947910 + base_args = [eden_cli] + self.get_required_eden_cli_args() + config_dir_args + start_cmd = base_args + ["start", "--daemon-binary", fake_edenfs] + stop_cmd = base_args + ["stop"] + subprocess.check_call(start_cmd) + try: + argv = get_fake_edenfs_argv(resolved_path) + self.assert_eden_dir(argv, resolved_path) + finally: + subprocess.call(stop_cmd) def assert_eden_dir(self, argv: List[str], expected: pathlib.Path) -> None: try: @@ -338,17 +364,14 @@ class StartFakeEdenFSTest(StartFakeEdenFSTestBase, PexpectAssertionMixin): def test_eden_start_fails_if_edenfs_is_already_running(self) -> None: with self.spawn_fake_edenfs(self.eden_dir) as daemon_pid: - start_process = self.spawn_start() - start_process.expect_exact(f"EdenFS is already running (pid {daemon_pid})") - self.assert_process_fails(start_process) + self.expect_start_failure(f"EdenFS is already running (pid {daemon_pid})") def test_eden_start_fails_if_edenfs_fails_during_startup(self) -> None: - start_process = self.spawn_start(extra_args=["--", "--failDuringStartup"]) - start_process.expect_exact( + self.expect_start_failure( "Started successfully, but reporting failure because " - "--failDuringStartup was specified" + "--failDuringStartup was specified", + extra_args=["--", "--failDuringStartup"], ) - self.assert_process_fails(start_process, 1) class StartWithSystemdTest(SystemdServiceTest, StartFakeEdenFSTestBase): @@ -365,28 +388,27 @@ class StartWithSystemdTest(SystemdServiceTest, StartFakeEdenFSTestBase): service = self.get_edenfs_systemd_service(eden_dir=self.eden_dir) self.assertEqual(service.query_active_state(), "active") - start_process = self.spawn_start() - start_process.expect_exact( + proc = self.expect_start_failure( f"error: edenfs systemd service is already running" ) # edenfsctl should show the output of 'systemctl status'. - start_process.expect(r"\bfb-edenfs@.*?\.service\b") - start_process.expect(r"Active:[^\n]*active \(running\)") - self.assert_process_fails(start_process, 1) + self.assertRegex(proc.stdout, r"\bfb-edenfs@.*?\.service\b") + self.assertRegex(proc.stdout, r"Active:[^\n]*active \(running\)") +@contextlib.contextmanager def run_eden_start_with_real_daemon( eden_dir: pathlib.Path, etc_eden_dir: pathlib.Path, home_dir: pathlib.Path, systemd: bool, -) -> None: +) -> Generator[None, None, None]: env = dict(os.environ) if systemd: env["EDEN_EXPERIMENTAL_SYSTEMD"] = "1" else: env.pop("EDEN_EXPERIMENTAL_SYSTEMD", None) - command: List[str] = [ # pyre-ignore[9]: T38947910 + eden_cli_args: List[str] = [ # pyre-ignore[9]: T38947910 FindExe.EDEN_CLI, "--config-dir", str(eden_dir), @@ -394,13 +416,21 @@ def run_eden_start_with_real_daemon( str(etc_eden_dir), "--home-dir", str(home_dir), + ] + + start_cmd: List[str] = eden_cli_args + [ # pyre-ignore[6]: T38947910 "start", "--daemon-binary", FindExe.EDEN_DAEMON, ] if eden_start_needs_allow_root_option(systemd=systemd): - command.extend(["--", "--allowRoot"]) - subprocess.check_call(command, env=env) + start_cmd.extend(["--", "--allowRoot"]) + subprocess.check_call(start_cmd, env=env) + + yield + + stop_cmd = eden_cli_args + ["stop"] + subprocess.check_call(stop_cmd, env=env) def eden_start_needs_allow_root_option(systemd: bool) -> bool: