Repository: mystor/git-revise (language: python)

Issue:
test.edit: Async editor
This simplifies and fixes some race conditions in the editing server by introducing a blocking queue of pending edit requests.

This means that if the launched `git-revise` process invokes the editor before the test invokes `next_file()`, the request will still be picked up from the queue. Previously, this relied on checking an attribute `server.current` which was set in another thread.

This also splits apart `EditorFile` (the mutable/inspectable edit data that the test can manipulate) from the HTTP request handler itself. Beyond separation of those concerns, a side effect is that this fixes pylint's ability to disentangle the expected type of the `next_file` context manager. Previously the mutual modification between `Editor` and `EditorFile` previously forced it to give up. (Although pylint is not enabled in `tests/` in this branch, it is in another).


~Merge resolutions:~
  * ~The resolved  merge of this branch, #116 (_test + pylint…_), and #117 (_test + mypy…_) can be found at [rwe/git-revise@test-mypy-pylint-async-editor](https://github.com/mystor/git-revise/compare/main...rwe:test-mypy-pylint-async-editor?expand=1).~


Discussion / hints:

Fix patch:
diff --git a/tox.ini b/tox.ini
index 0322109..ad21b69 100644
--- a/tox.ini
+++ b/tox.ini
@@ -22,9 +22,11 @@ passenv = PROGRAMFILES*  # to locate git-bash on windows
 
 [testenv:mypy]
 description = typecheck with mypy
-commands = mypy gitrevise {posargs}
+commands = mypy gitrevise tests {posargs}
 basepython = python3.10
-deps = mypy
+deps =
+    {[testenv]deps}
+    mypy
 
 [testenv:lint]
 description = lint with pylint


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index fb23c3c..e8f8d96 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -9,6 +9,18 @@
 import textwrap
 import subprocess
 import traceback
+from types import TracebackType
+from typing import (
+    Any,
+    Callable,
+    Generator,
+    Optional,
+    Sequence,
+    Tuple,
+    Type,
+    Union,
+    TYPE_CHECKING,
+)
 from gitrevise.odb import Repository
 from gitrevise.utils import sh_path
 from contextlib import contextmanager
@@ -17,8 +29,15 @@
 import dummy_editor
 
 
+if TYPE_CHECKING:
+    from _typeshed import StrPath
+
+
 @pytest.fixture(autouse=True)
-def hermetic_seal(tmp_path_factory, monkeypatch):
+def hermetic_seal(
+    tmp_path_factory: pytest.TempPathFactory,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
     # Lock down user git configuration
     home = tmp_path_factory.mktemp("home")
     xdg_config_home = home / ".config"
@@ -59,23 +78,27 @@ def hermetic_seal(tmp_path_factory, monkeypatch):
 
 
 @pytest.fixture
-def repo(hermetic_seal):
+def repo(hermetic_seal: None) -> Generator[Repository, None, None]:
     with Repository() as repo:
         yield repo
 
 
 @pytest.fixture
-def short_tmpdir():
+def short_tmpdir() -> Generator[py.path.local, None, None]:
     with tempfile.TemporaryDirectory() as tdir:
         yield py.path.local(tdir)
 
 
 @contextmanager
-def in_parallel(func, *args, **kwargs):
+def in_parallel(
+    func: Callable[..., Any],
+    *args: Any,
+    **kwargs: Any,
+) -> "Generator[None, None, None]":
     class HelperThread(Thread):
-        exception = None
+        exception: Optional[Exception] = None
 
-        def run(self):
+        def run(self) -> None:
             try:
                 func(*args, **kwargs)
             except Exception as exc:
@@ -93,7 +116,7 @@ def run(self):
         raise thread.exception
 
 
-def bash(command):
+def bash(command: str) -> None:
     # Use a custom environment for bash commands so commits with those commands
     # have unique names and emails.
     env = dict(
@@ -106,7 +129,7 @@ def bash(command):
     subprocess.run([sh_path(), "-ec", textwrap.dedent(command)], check=True, env=env)
 
 
-def changeline(path, lineno, newline):
+def changeline(path: "StrPath", lineno: int, newline: bytes) -> None:
     with open(path, "rb") as f:
         lines = f.readlines()
     lines[lineno] = newline
@@ -115,15 +138,23 @@ def changeline(path, lineno, newline):
 
 
 # Run the main entry point for git-revise in a subprocess.
-def main(args, **kwargs):
-    kwargs.setdefault("check", True)
+def main(
+    args: Sequence[str],
+    cwd: Optional["StrPath"] = None,
+    input: Optional[bytes] = None,
+    check: bool = True,
+) -> "subprocess.CompletedProcess[bytes]":
     cmd = [sys.executable, "-m", "gitrevise", *args]
-    print("Running", cmd, kwargs)
-    return subprocess.run(cmd, **kwargs)
+    print("Running", cmd, dict(cwd=cwd, input=input, check=check))
+    return subprocess.run(cmd, cwd=cwd, input=input, check=check)
 
 
 @contextmanager
-def editor_main(args, **kwargs):
+def editor_main(
+    args: Sequence[str],
+    cwd: Optional["StrPath"] = None,
+    input: Optional[bytes] = None,
+) -> "Generator[Editor, None, None]":
     with pytest.MonkeyPatch().context() as m, Editor() as ed:
         editor_cmd = " ".join(
             shlex.quote(p)
@@ -135,11 +166,12 @@ def editor_main(args, **kwargs):
         )
         m.setenv("GIT_EDITOR", editor_cmd)
 
-        def main_wrapper():
+        def main_wrapper() -> Optional["subprocess.CompletedProcess[bytes]"]:
             try:
-                return main(args, **kwargs)
+                return main(args, cwd=cwd, input=input)
             except Exception as e:
                 ed.exception = e
+                return None
             finally:
                 if not ed.exception:
                     ed.exception = Exception(
@@ -152,14 +184,23 @@ def main_wrapper():
 
 
 class EditorFile(BaseHTTPRequestHandler):
-    def __init__(self, *args, **kwargs):
+    indata: Optional[bytes]
+    outdata: Optional[bytes]
+    server: "Editor"
+
+    def __init__(
+        self,
+        request: bytes,
+        client_address: Tuple[str, int],
+        server: "Editor",
+    ) -> None:
         self.response_ready = Event()
         self.indata = None
         self.outdata = None
         self.exception = None
-        super().__init__(*args, **kwargs)
+        super().__init__(request=request, client_address=client_address, server=server)
 
-    def do_POST(self):
+    def do_POST(self) -> None:
         length = int(self.headers.get("content-length"))
         self.indata = self.rfile.read(length)
         self.outdata = b""
@@ -174,41 +215,49 @@ def do_POST(self):
         finally:
             self.server.current = None
 
-    def send_editor_reply(self, status, data):
+    def send_editor_reply(self, status: int, data: bytes) -> None:
         assert not self.response_ready.is_set(), "already replied?"
         self.send_response(status)
-        self.send_header("content-length", len(data))
+        self.send_header("content-length", str(len(data)))
         self.end_headers()
         self.wfile.write(data)
         self.response_ready.set()
 
         # Ensure the handle thread has shut down
-        self.server.handle_thread.join()
-        self.server.handle_thread = None
+        if self.server.handle_thread is not None:
+            self.server.handle_thread.join()
+            self.server.handle_thread = None
         assert self.server.current is None
 
-    def startswith(self, text):
+    def startswith(self, text: bytes) -> bool:
+        assert self.indata is not None
         return self.indata.startswith(text)
 
-    def startswith_dedent(self, text):
+    def startswith_dedent(self, text: str) -> bool:
         return self.startswith(textwrap.dedent(text).encode())
 
-    def equals(self, text):
+    def equals(self, text: bytes) -> bool:
         return self.indata == text
 
-    def equals_dedent(self, text):
+    def equals_dedent(self, text: str) -> bool:
         return self.equals(textwrap.dedent(text).encode())
 
-    def replace_dedent(self, text):
+    def replace_dedent(self, text: Union[str, bytes]) -> None:
         if isinstance(text, str):
             text = textwrap.dedent(text).encode()
         self.outdata = text
 
-    def __enter__(self):
+    def __enter__(self) -> "EditorFile":
         return self
 
-    def __exit__(self, etype, evalue, tb):
+    def __exit__(
+        self,
+        etype: Optional[Type[BaseException]],
+        evalue: Optional[BaseException],
+        tb: Optional[TracebackType],
+    ) -> None:
         if etype is None:
+            assert self.outdata
             self.send_editor_reply(200, self.outdata)
         else:
             exc = "".join(traceback.format_exception(etype, evalue, tb)).encode()
@@ -217,12 +266,18 @@ def __exit__(self, etype, evalue, tb):
             except:
                 pass
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return f"<EditorFile {self.indata!r}>"
 
 
 class Editor(HTTPServer):
-    def __init__(self):
+    request_ready: Event
+    handle_thread: Optional[Thread]
+    current: Optional[EditorFile]
+    exception: Optional[Exception]
+    timeout: int
+
+    def __init__(self) -> None:
         # Bind to a randomly-allocated free port.
         super().__init__(("127.0.0.1", 0), EditorFile)
         self.request_ready = Event()
@@ -231,7 +286,7 @@ def __init__(self):
         self.exception = None
         self.timeout = 10
 
-    def next_file(self):
+    def next_file(self) -> EditorFile:
         assert self.handle_thread is None
         assert self.current is None
 
@@ -249,13 +304,18 @@ def next_file(self):
         assert self.current
         return self.current
 
-    def is_idle(self):
+    def is_idle(self) -> bool:
         return self.handle_thread is None and self.current is None
 
-    def __enter__(self):
+    def __enter__(self) -> "Editor":
         return self
 
-    def __exit__(self, etype, value, tb):
+    def __exit__(
+        self,
+        etype: Optional[Type[BaseException]],
+        value: Optional[BaseException],
+        tb: Optional[TracebackType],
+    ) -> None:
         try:
             # Only assert if we're not already raising an exception.
             if etype is None:
diff --git a/tests/test_cut.py b/tests/test_cut.py
index df7842f..3a3846c 100644
--- a/tests/test_cut.py
+++ b/tests/test_cut.py
@@ -3,7 +3,7 @@
 from conftest import *
 
 
-def test_cut(repo):
+def test_cut(repo: Repository) -> None:
     bash(
         """
         echo "Hello, World" >> file1
@@ -46,7 +46,7 @@ def test_cut(repo):
     assert new_uu == prev_uu
 
 
-def test_cut_root(repo):
+def test_cut_root(repo: Repository) -> None:
     bash(
         """
         echo "Hello, World" >> file1
diff --git a/tests/test_fixup.py b/tests/test_fixup.py
index 409a3e8..c206a69 100644
--- a/tests/test_fixup.py
+++ b/tests/test_fixup.py
@@ -7,7 +7,7 @@
 
 
 @pytest.fixture
-def basic_repo(repo):
+def basic_repo(repo: Repository) -> Repository:
     bash(
         """
         cat <<EOF >file1
@@ -29,13 +29,23 @@ def basic_repo(repo):
     return repo
 
 
-def fixup_helper(repo, flags, target, message=None):
+def fixup_helper(
+    repo: Repository,
+    flags: Sequence[str],
+    target: str,
+    message: Optional[str] = None,
+) -> None:
     with fixup_helper_editor(repo=repo, flags=flags, target=target, message=message):
         pass
 
 
 @contextmanager
-def fixup_helper_editor(repo, flags, target, message=None):
+def fixup_helper_editor(
+    repo: Repository,
+    flags: Sequence[str],
+    target: str,
+    message: Optional[str] = None,
+) -> Generator[Editor, None, None]:
     old = repo.get_commit(target)
     assert old.persisted
 
@@ -46,7 +56,7 @@ def fixup_helper_editor(repo, flags, target, message=None):
         """
     )
 
-    with editor_main(flags + [target]) as ed:
+    with editor_main((*flags, target)) as ed:
         yield ed
 
     new = repo.get_commit(target)
@@ -65,15 +75,15 @@ def fixup_helper_editor(repo, flags, target, message=None):
     assert new.committer == repo.default_committer, "committer is updated"
 
 
-def test_fixup_head(basic_repo):
+def test_fixup_head(basic_repo: Repository) -> None:
     fixup_helper(basic_repo, [], "HEAD")
 
 
-def test_fixup_nonhead(basic_repo):
+def test_fixup_nonhead(basic_repo: Repository) -> None:
     fixup_helper(basic_repo, [], "HEAD~")
 
 
-def test_fixup_head_msg(basic_repo):
+def test_fixup_head_msg(basic_repo: Repository) -> None:
     fixup_helper(
         basic_repo,
         ["-m", "fixup_head test", "-m", "another line"],
@@ -82,7 +92,7 @@ def test_fixup_head_msg(basic_repo):
     )
 
 
-def test_fixup_nonhead_msg(basic_repo):
+def test_fixup_nonhead_msg(basic_repo: Repository) -> None:
     fixup_helper(
         basic_repo,
         ["-m", "fixup_nonhead test", "-m", "another line"],
@@ -91,7 +101,7 @@ def test_fixup_nonhead_msg(basic_repo):
     )
 
 
-def test_fixup_head_editor(basic_repo):
+def test_fixup_head_editor(basic_repo: Repository) -> None:
     old = basic_repo.get_commit("HEAD")
     newmsg = "fixup_head_editor test\n\nanother line\n"
 
@@ -101,7 +111,7 @@ def test_fixup_head_editor(basic_repo):
             f.replace_dedent(newmsg)
 
 
-def test_fixup_nonhead_editor(basic_repo):
+def test_fixup_nonhead_editor(basic_repo: Repository) -> None:
     old = basic_repo.get_commit("HEAD~")
     newmsg = "fixup_nonhead_editor test\n\nanother line\n"
 
@@ -111,7 +121,7 @@ def test_fixup_nonhead_editor(basic_repo):
             f.replace_dedent(newmsg)
 
 
-def test_fixup_nonhead_conflict(basic_repo):
+def test_fixup_nonhead_conflict(basic_repo: Repository) -> None:
     bash('echo "conflict" > file1')
     bash("git add file1")
 
@@ -151,7 +161,7 @@ def test_fixup_nonhead_conflict(basic_repo):
     assert new != old
 
 
-def test_autosquash_nonhead(repo):
+def test_autosquash_nonhead(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -195,7 +205,7 @@ def test_autosquash_nonhead(repo):
     assert file2 == b"second file\nextra line\n"
 
 
-def test_fixup_of_fixup(repo):
+def test_fixup_of_fixup(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -243,7 +253,7 @@ def test_fixup_of_fixup(repo):
     assert file2 == b"second file\nextra line\neven more\n"
 
 
-def test_fixup_by_id(repo):
+def test_fixup_by_id(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -287,7 +297,7 @@ def test_fixup_by_id(repo):
     assert file2 == b"second file\nextra line\n"
 
 
-def test_fixup_order(repo):
+def test_fixup_order(repo: Repository) -> None:
     bash(
         """
         git commit --allow-empty -m 'old'
@@ -310,7 +320,7 @@ def test_fixup_order(repo):
     assert b"second fixup" in second.commit.message
 
 
-def test_fixup_order_transitive(repo):
+def test_fixup_order_transitive(repo: Repository) -> None:
     bash(
         """
         git commit --allow-empty -m 'old'
@@ -335,7 +345,7 @@ def test_fixup_order_transitive(repo):
     assert b"2.0" in c.commit.message
 
 
-def test_fixup_order_cycle(repo):
+def test_fixup_order_cycle(repo: Repository) -> None:
     bash(
         """
         git commit --allow-empty -m 'old'
@@ -357,7 +367,7 @@ def test_fixup_order_cycle(repo):
     assert all(step.kind == StepKind.PICK for step in new_todos)
 
 
-def test_autosquash_multiline_summary(repo):
+def test_autosquash_multiline_summary(repo: Repository) -> None:
     bash(
         """
         git commit --allow-empty -m 'initial commit'
diff --git a/tests/test_gpgsign.py b/tests/test_gpgsign.py
index c1ebf6a..ea65aeb 100644
--- a/tests/test_gpgsign.py
+++ b/tests/test_gpgsign.py
@@ -5,7 +5,11 @@
 from gitrevise.utils import sh_run
 
 
-def test_gpgsign(repo, short_tmpdir, monkeypatch):
+def test_gpgsign(
+    repo: Repository,
+    short_tmpdir: py.path.local,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
     bash("git commit --allow-empty -m 'commit 1'")
     assert repo.get_commit("HEAD").gpgsig is None
 
diff --git a/tests/test_interactive.py b/tests/test_interactive.py
index d9431cf..3c70394 100644
--- a/tests/test_interactive.py
+++ b/tests/test_interactive.py
@@ -4,7 +4,7 @@
 from conftest import *
 
 
-def interactive_reorder_helper(repo, cwd):
+def interactive_reorder_helper(repo: Repository, cwd: "StrPath") -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -58,16 +58,16 @@ def interactive_reorder_helper(repo, cwd):
     assert prev.tree().entries[b"file1"] == curr_u.tree().entries[b"file1"]
 
 
-def test_interactive_reorder(repo):
+def test_interactive_reorder(repo: Repository) -> None:
     interactive_reorder_helper(repo, cwd=repo.workdir)
 
 
-def test_interactive_reorder_subdir(repo):
+def test_interactive_reorder_subdir(repo: Repository) -> None:
     bash("mkdir subdir")
     interactive_reorder_helper(repo, cwd=repo.workdir / "subdir")
 
 
-def test_interactive_on_root(repo):
+def test_interactive_on_root(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -126,7 +126,7 @@ def test_interactive_on_root(repo):
     assert new_commit3.tree() == orig_commit3.tree()
 
 
-def test_interactive_fixup(repo):
+def test_interactive_fixup(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -210,7 +210,12 @@ def test_interactive_fixup(repo):
         (None, "1", True),
     ],
 )
-def test_autosquash_config(repo, rebase_config, revise_config, expected):
+def test_autosquash_config(
+    repo: Repository,
+    rebase_config: Optional[str],
+    revise_config: Optional[str],
+    expected: bool,
+) -> None:
     bash(
         """
         echo "hello, world" > file1
@@ -253,8 +258,8 @@ def test_autosquash_config(repo, rebase_config, revise_config, expected):
 
         """
 
-    def subtest(args, expected_todos):
-        with editor_main(args + ["-i", "HEAD~3"]) as ed:
+    def subtest(args: Sequence[str], expected_todos: str) -> None:
+        with editor_main((*args, "-i", "HEAD~3")) as ed:
             with ed.next_file() as f:
                 assert f.startswith_dedent(expected_todos)
                 f.replace_dedent(disabled)  # don't mutate state
@@ -266,7 +271,7 @@ def subtest(args, expected_todos):
     subtest(["--no-autosquash"], disabled)
 
 
-def test_interactive_reword(repo):
+def test_interactive_reword(repo: Repository) -> None:
     bash(
         """
         echo "hello, world" > file1
diff --git a/tests/test_rerere.py b/tests/test_rerere.py
index 582afed..d2709eb 100644
--- a/tests/test_rerere.py
+++ b/tests/test_rerere.py
@@ -6,7 +6,7 @@
 from gitrevise.merge import normalize_conflicted_file
 
 
-def history_with_two_conflicting_commits(autoUpdate: bool = False):
+def history_with_two_conflicting_commits(autoUpdate: bool = False) -> None:
     bash(
         f"""
         git config rerere.enabled true
@@ -18,7 +18,7 @@ def history_with_two_conflicting_commits(autoUpdate: bool = False):
     )
 
 
-def test_reuse_recorded_resolution(repo):
+def test_reuse_recorded_resolution(repo: Repository) -> None:
     history_with_two_conflicting_commits(autoUpdate=True)
 
     with editor_main(("-i", "HEAD~~"), input=b"y\ny\ny\ny\n") as ed:
@@ -56,7 +56,7 @@ def test_reuse_recorded_resolution(repo):
             f.replace_dedent("resolved one\n")
 
 
-def test_rerere_no_autoupdate(repo):
+def test_rerere_no_autoupdate(repo: Repository) -> None:
     history_with_two_conflicting_commits(autoUpdate=False)
 
     with editor_main(("-i", "HEAD~~"), input=b"y\ny\ny\ny\n") as ed:
@@ -96,7 +96,7 @@ def test_rerere_no_autoupdate(repo):
     )
 
 
-def test_rerere_merge(repo):
+def test_rerere_merge(repo: Repository) -> None:
     (repo.workdir / "file").write_bytes(10 * b"x\n")
     bash(
         """
@@ -150,7 +150,7 @@ def test_rerere_merge(repo):
     )
 
 
-def test_replay_resolution_recorded_by_git(repo):
+def test_replay_resolution_recorded_by_git(repo: Repository) -> None:
     history_with_two_conflicting_commits(autoUpdate=True)
     # Switch the order of the last two commits, recording the conflict
     # resolution with Git itself.
@@ -218,7 +218,7 @@ def test_replay_resolution_recorded_by_git(repo):
     )
 
 
-def test_normalize_conflicted_file():
+def test_normalize_conflicted_file() -> None:
     # Normalize conflict markers and labels.
     assert normalize_conflicted_file(
         dedent(
@@ -346,9 +346,10 @@ def test_normalize_conflicted_file():
     )
 
 
-def flip_last_two_commits(repo: Repository, ed: Editor):
+def flip_last_two_commits(repo: Repository, ed: Editor) -> None:
     head = repo.get_commit("HEAD")
     with ed.next_file() as f:
+        assert f.indata
         lines = f.indata.splitlines()
         assert lines[0].startswith(b"pick " + head.parent().oid.short().encode())
         assert lines[1].startswith(b"pick " + head.oid.short().encode())
diff --git a/tests/test_reword.py b/tests/test_reword.py
index 39b8a16..ce571c6 100644
--- a/tests/test_reword.py
+++ b/tests/test_reword.py
@@ -6,7 +6,7 @@
 
 @pytest.mark.parametrize("target", ["HEAD", "HEAD~", "HEAD~~"])
 @pytest.mark.parametrize("use_editor", [True, False])
-def test_reword(repo, target, use_editor):
+def test_reword(repo: Repository, target: str, use_editor: bool) -> None:
     bash(
         """
         echo "hello, world" > file1