(only) add more unit tests (#76)

* (only) add more unit tests Signed-off-by: Fabrice Normandin <[email protected]> * Add nice test for transform-related methods Signed-off-by: Fabrice Normandin <[email protected]> * Remove/simplify a few tests Signed-off-by: Fabrice Normandin <[email protected]> * Remove duplicate tests Signed-off-by: Fabrice Normandin <[email protected]> * Greatly improved the tests Signed-off-by: Fabrice Normandin <[email protected]> * Mark tests that work/don't work with `-s` flag Signed-off-by: Fabrice Normandin <[email protected]> * Also show result of .run in test Signed-off-by: Fabrice Normandin <[email protected]> * Fix isort Signed-off-by: Fabrice Normandin <[email protected]> * Fix syntaxerror in test_remote.py Signed-off-by: Fabrice Normandin <[email protected]> * Remove f-string {val=} expression for py<3.9 Signed-off-by: Fabrice Normandin <[email protected]> * Fix check for `ssh localhost` being setup Signed-off-by: Fabrice Normandin <[email protected]> * Minor touchups Signed-off-by: Fabrice Normandin <[email protected]> * Fix display of result string in regression files Signed-off-by: Fabrice Normandin <[email protected]> * Also run pytest with `-s` flag in CI Signed-off-by: Fabrice Normandin <[email protected]> --------- Signed-off-by: Fabrice Normandin <[email protected]> Signed-off-by: Fabrice Normandin <[email protected]>
mila-iqia · Nov 27, 2023 · ed140e5 · ed140e5
1 parent e64e2dd
commit ed140e5
Show file tree

Hide file tree

Showing 13 changed files with 1,322 additions and 140 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -22,7 +22,7 @@ jobs:
       run: |
         python -m pip install --upgrade pip
         pip install poetry
-        poetry install
+        poetry install --with=dev
 
     - name: Check formatting with black
       if: ${{ matrix.python-version == '3.10' }}
@@ -34,3 +34,6 @@ jobs:
 
     - name: Test with pytest
       run: poetry run pytest
+
+    - name: Test with pytest (with -s flag)
+      run: poetry run pytest -s
diff --git a/poetry.lock b/poetry.lock
diff --git a/pyproject.toml b/pyproject.toml
@@ -13,8 +13,8 @@ python = "^3.7"
 blessed = "^1.18.1"
 sshconf = "^0.2.2"
 questionary = "^1.10.0"
-Fabric = "^2.7.0"
 typing-extensions = "^4.7.1"
+fabric = "^3.2.2"
 
 [tool.poetry.dev-dependencies]
 black = ">= 21.8b0"
@@ -33,6 +33,9 @@ mila = "milatools.cli.__main__:main"
 [tool.poetry.group.dev.dependencies]
 pytest = "^7.2.1"
 pytest-regressions = "^2.4.2"
+fabric = {extras = ["testing"], version = "^3.2.2"}
+pytest-mock = "^3.11.1"
+pytest-socket = "^0.6.0"
 
 [tool.isort]
 multi_line_output = 3
@@ -42,6 +45,7 @@ combine_as_imports = true
 
 [tool.pytest.ini_options]
 addopts = "--doctest-modules"
+markers = "--enable-internet: Allow some tests to run using real connections to the cluster."
 
 [build-system]
 requires = ["poetry-core>=1.0.0"]

diff --git a/tests/cli/common.py b/tests/cli/common.py
@@ -1,4 +1,36 @@
+from __future__ import annotations
+
+import inspect
+import sys
+import typing
 from subprocess import CompletedProcess
+from typing import Any, Callable
+
+import pytest
+from pytest_regressions.file_regression import FileRegressionFixture
+from typing_extensions import ParamSpec
+
+if typing.TYPE_CHECKING:
+    from typing_extensions import TypeGuard
+
+
+P = ParamSpec("P")
+
+
+requires_s_flag = pytest.mark.skipif(
+    "-s" not in sys.argv,
+    reason=(
+        "Seems to require reading from stdin? Works with the -s flag, but other "
+        "tests might not."
+    ),
+)
+
+
+requires_no_s_flag = pytest.mark.skipif(
+    "-s" in sys.argv,
+    reason="Passing pytest's -s flag makes this test fail.",
+)
+
 
 cmdtest = """===============
 Captured stdout
@@ -19,7 +51,14 @@
 """
 
 
-def output_tester(func, capsys, file_regression):
+def output_tester(
+    func: Callable[
+        [],
+        tuple[str | CompletedProcess[str] | None, str | CompletedProcess[str] | None],
+    ],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     out, err = None, None
     try:
         out, err = func()
@@ -32,3 +71,70 @@ def output_tester(func, capsys, file_regression):
         file_regression.check(
             cmdtest.format(cout=captured.out, cerr=captured.err, out=out, err=err)
         )
+
+
+def function_call_string(
+    fn: Callable[P, Any],
+    *args: P.args,
+    **kwargs: P.kwargs,
+) -> str:
+    """Returns a nice string representation of code that calls `fn(*args, **kwargs)`.
+
+    This is used to show code snippets in the regression files generated by unit tests.
+    """
+
+    # Call `repr` on the arguments, except for lambdas, which are shown as their body.
+    args_str = [_lambda_to_str(v) if _is_lambda(v) else repr(v) for v in args]
+    kwargs_str = {
+        k: _lambda_to_str(v) if _is_lambda(v) else repr(v) for k, v in kwargs.items()
+    }
+    fn_str = fn.__name__
+
+    single_line = (
+        fn_str
+        + "("
+        + ", ".join(args_str)
+        + (", " if args_str and kwargs_str else "")
+        + ", ".join(f"{k}={v}" for k, v in kwargs_str.items())
+        + ")"
+    )
+    indent = 4 * " "
+    multi_line = (
+        f"{fn_str}(\n"
+        + "\n".join(f"{indent}{arg}," for arg in args_str)
+        + ("\n" if args_str and kwargs_str else "")
+        + "\n".join(f"{indent}{key}={value}," for key, value in kwargs_str.items())
+        + "\n)"
+    )
+
+    if len(single_line) < 80:
+        return single_line
+    return multi_line
+
+
+def _is_lambda(v: Any) -> TypeGuard[Callable]:
+    """Returns whether the value is a lambda expression."""
+    return (
+        callable(v)
+        and isinstance(v, type(lambda _: _))
+        and getattr(v, "__name__", None) == "<lambda>"
+    )
+
+
+def _lambda_to_str(lambda_: Callable) -> str:
+    """Shows the body of the lambda instead of the default repr."""
+    lambda_body = inspect.getsource(lambda_).strip()
+    # when putting lambdas in a list, like so:
+    # funcs = [
+    #    lambda x: x + 1,
+    # ]
+    # a trailing comma is returned by `inspect.getsource`, which we want to remove.
+    return _removesuffix(lambda_body, ",")
+
+
+def _removesuffix(s: str, suffix: str) -> str:
+    """Backport of `str.removesuffix` for Python<3.9."""
+    if s.endswith(suffix):
+        return s[: -len(suffix)]
+    else:
+        return s
diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py
@@ -1,18 +1,73 @@
 import contextlib
+import importlib
 import io
 import shlex
+import subprocess
+from subprocess import CompletedProcess
+from unittest import mock
 
 import pytest
+from fabric.testing.fixtures import Connection, MockRemote, remote  # noqa
+from pytest_mock import MockerFixture
 from pytest_regressions.file_regression import FileRegressionFixture
+from typing_extensions import ParamSpec
 
 from milatools.cli.commands import main
+from milatools.cli.local import Local
+
+from .common import requires_no_s_flag
 
 
 def _convert_argparse_output_to_pre_py311_format(output: str) -> str:
-    """Revert the slight change in the output format of argparse in python 3.11."""
+    """Revert the slight change in the output of argparse in python 3.11."""
     return output.replace("options:\n", "optional arguments:\n")
 
 
+P = ParamSpec("P")
+
+
+def reload_module():
+    """Reload the module after mocking out functions.
+
+    Need to reload the module because we have mocked some of the functions that are used
+    as default values in the methods of the class under test (e.g. `subprocess.run`).
+    The functions being in the signature of the methods makes it possible for us to
+    describe their our method signatures explicitly, but it means that we need to reload
+    the module after mocking them.
+    """
+    global Local
+    import milatools.cli.local
+
+    importlib.reload(milatools.cli.local)
+    Local = milatools.cli.local.Local
+
+
+@pytest.fixture
+def mock_subprocess_run(mocker: MockerFixture) -> mock.Mock:
+    mock_subprocess_run: mock.Mock = mocker.patch("subprocess.run")
+    # NOTE: Trying out https://stackoverflow.com/questions/25692440/mocking-a-subprocess-call-in-python
+
+    reload_module()
+    return mock_subprocess_run
+
+
+def test_check_passwordless(
+    mock_subprocess_run: mock.Mock,
+):
+    # NOTE: Trying out https://stackoverflow.com/questions/25692440/mocking-a-subprocess-call-in-python
+    mock_subprocess_run.return_value = CompletedProcess(["echo OK"], 0, stdout="BOBOBO")
+    local = Local()
+    local.check_passwordless("mila")
+
+    mock_subprocess_run.assert_called_once_with(
+        tuple(shlex.split("ssh -oPreferredAuthentications=publickey mila 'echo OK'")),
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        universal_newlines=True,
+    )
+
+
+@requires_no_s_flag
 @pytest.mark.parametrize(
     "command",
     ["mila"]
@@ -47,11 +102,11 @@ def test_help(
     ), contextlib.redirect_stderr(buf):
         with pytest.raises(SystemExit):
             main()
-
     output: str = buf.getvalue()
     file_regression.check(_convert_argparse_output_to_pre_py311_format(output))
 
 
+@requires_no_s_flag
 @pytest.mark.parametrize(
     "command",
     [
@@ -86,7 +141,7 @@ def test_invalid_command_output(
 #         *args,
 #         marks=pytest.mark.skipif(
 #             "GITHUB_ACTIONS" in os.environ,
-#             reason="We don't run this test on GitHub Actions for security reasons.",
+#             reason="We don't run this test on GitHub Actions.",
 #         ),
 #     )
 

diff --git a/tests/cli/test_local.py b/tests/cli/test_local.py
@@ -1,33 +1,55 @@
+from __future__ import annotations
+
 from subprocess import PIPE
 
 import pytest
+from pytest_regressions.file_regression import FileRegressionFixture
 
 from milatools.cli.local import CommandNotFoundError, Local
 
-from .common import output_tester
+from .common import output_tester, requires_no_s_flag
 
 _ECHO_CMD = ["echo", "--arg1", "val1", "--arg2=val2", "X"]
 _FAKE_CMD = ["FAKEcmd", "--arg1", "val1", "--arg2=val2", "X"]
 _FAIL_CODE_CMD = ["FAKEcode", "--arg1", "val1", "--arg2=val2", "X"]
 
 
+@requires_no_s_flag
 @pytest.mark.parametrize("cmd", [_ECHO_CMD, _FAKE_CMD])
-def test_display(cmd, capsys, file_regression):
+def test_display(
+    cmd: list[str],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     output_tester(lambda: (Local().display(cmd), None), capsys, file_regression)
 
 
 @pytest.mark.parametrize("cmd", [_ECHO_CMD])
-def test_silent_get(cmd, capsys, file_regression):
+def test_silent_get(
+    cmd: list[str],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     output_tester(lambda: (Local().silent_get(*cmd), None), capsys, file_regression)
 
 
+@requires_no_s_flag
 @pytest.mark.parametrize("cmd", [_ECHO_CMD])
-def test_get(cmd, capsys, file_regression):
+def test_get(
+    cmd: list[str],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     output_tester(lambda: (Local().get(*cmd), None), capsys, file_regression)
 
 
+@requires_no_s_flag
 @pytest.mark.parametrize("cmd", [_ECHO_CMD, _FAKE_CMD, _FAIL_CODE_CMD])
-def test_run(cmd, capsys, file_regression):
+def test_run(
+    cmd: list[str],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     def func():
         return Local().run(*cmd, capture_output=True), None
 
@@ -47,8 +69,13 @@ def _catch_exc():
         output_tester(func, capsys, file_regression)
 
 
+@requires_no_s_flag
 @pytest.mark.parametrize("cmd", [_ECHO_CMD])
-def test_popen(cmd, capsys, file_regression):
+def test_popen(
+    cmd: list[str],
+    capsys: pytest.CaptureFixture,
+    file_regression: FileRegressionFixture,
+):
     output_tester(
         lambda: Local().popen(*cmd, stdout=PIPE, stderr=PIPE).communicate(),
         capsys,