clean up parts of the start and clone integration tests

Summary:
Several of the tests started edenfs of fake_edenfs processes and didn't kill
them at the end of the test.  This adds proper cleanup functions to try and
kill these processes when the test exits.

This also eliminates using pexpect in these tests, since it isn't actually
necessary here.

Reviewed By: wez

Differential Revision: D21084096

fbshipit-source-id: 4e92a99a5c398d4a78830ac51507ba34d7a6c0b1

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)

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())

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: