refactor for better test coverage

action/deploy_recipe.py

@@ -7,7 +7,8 @@
 import requests
 
 
-def deploy_recipe_cmd(cmd: list[str]):
+def call_subprocess_run(cmd: list[str]) -> str:
+    """Convenience wrapper for `subprocess.run` with stdout/stderr handling."""
     print(f"Calling subprocess with {cmd = }")
     submit_proc = subprocess.run(cmd, capture_output=True)
     stdout = submit_proc.stdout.decode()
@@ -18,21 +19,17 @@ def deploy_recipe_cmd(cmd: list[str]):
     if submit_proc.returncode != 0:
         for line in stderr.splitlines():
             print(line)
-        raise ValueError("Job submission failed.")
-    else:
-        lastline = json.loads(stdout.splitlines()[-1])
-        job_id = lastline["job_id"]
-        job_name = lastline["job_name"]
-        print(f"Job submitted with {job_id = } and {job_name = }")
+        raise ValueError(f"{cmd = } failed. See logging for details.")
+    return stdout
 
 
-def pip_install(requirements_file_path: str, conda_env: str):
-    print(f"Installing extra packages from {requirements_file_path}...")
-    install_cmd = f"mamba run -n {conda_env} pip install -Ur {requirements_file_path}".split()
-    install_proc = subprocess.run(install_cmd, capture_output=True, text=True)
-    if install_proc.returncode != 0:
-        # installations failed, so record the error and bail early
-        raise ValueError(f"Installs failed with {install_proc.stderr = }")
+def deploy_recipe_cmd(cmd: list[str]):
+    """Wrapper for `call_subprocess_run` with extra stdout parsing when deploying recipes."""
+    stdout = call_subprocess_run(cmd)
+    lastline = json.loads(stdout.splitlines()[-1])
+    job_id = lastline["job_id"]
+    job_name = lastline["job_name"]
+    print(f"Job submitted with {job_id = } and {job_name = }")
 
 
 def main():
@@ -92,7 +89,9 @@ def main():
     # working directory is the root of the feedstock repo, so we can list feedstock repo
     # contents directly on the filesystem here, without requesting it from github.
     if "requirements.txt" in os.listdir(feedstock_subdir):
-        pip_install(f"{feedstock_subdir}/requirements.txt", conda_env)
+        call_subprocess_run(
+            f"mamba run -n {conda_env} pip install -Ur {feedstock_subdir}/requirements.txt".split()
+        )
 
     with tempfile.NamedTemporaryFile("w", suffix=".json") as f:
         json.dump(config, f)

tests/test_main.py

@@ -1,5 +1,7 @@
+import json
 import os
 import sys
+from dataclasses import dataclass
 from pathlib import Path
 from unittest.mock import MagicMock, patch
 
@@ -22,8 +24,17 @@ def select_recipe_by_label(request):
     return request.param
 
 
+@pytest.fixture(params=["config", "broken-reqs"])
+def pangeo_forge_runner_config(request):
+    if request.param == "broken-reqs":
+        # TODO: possibly improve this, it's an awkward way to parametrize a broken requirements.txt
+        return json.dumps({"BaseCommand": {"feedstock-subdir": "broken-requirements-feedstock"}})
+    else:
+        return '{"a": "b"}'
+
+
 @pytest.fixture
-def env(select_recipe_by_label, head_ref):
+def env(select_recipe_by_label, head_ref, pangeo_forge_runner_config):
     return {
         "CONDA_ENV": "notebook",
         "GITHUB_REPOSITORY": "my/repo",
@@ -35,8 +46,7 @@ def env(select_recipe_by_label, head_ref):
         "GITHUB_REPOSITORY_ID": "1234567890",
         "GITHUB_RUN_ID": "0987654321",
         "GITHUB_RUN_ATTEMPT": "1",
-        # TODO: parametrize runner config with `BaseCommand.feedstock-subdir`
-        "INPUT_PANGEO_FORGE_RUNNER_CONFIG": '{"a": "b"}',
+        "INPUT_PANGEO_FORGE_RUNNER_CONFIG": pangeo_forge_runner_config,
         "INPUT_SELECT_RECIPE_BY_LABEL": select_recipe_by_label,
     }
 
@@ -48,24 +58,39 @@ def requests_get_returns_json():
     ]
 
 
+@dataclass
+class MockCompletedProcess:
+    stdout: bytes
+    stderr: bytes
+    returncode: int
+
+
 @pytest.fixture
-def subprocess_return_values():
-    return dict(
-        stdout='{"job_id": "foo", "job_name": "bar"}',
-        stderr="",
-        returncode=0,
-    )
+def subprocess_run_side_effect():
+    def _get_proc(cmd: list[str], *args, **kwargs):
+        if "broken-requirements-feedstock" in " ".join(cmd):
+            return MockCompletedProcess(
+                stdout=b"",
+                stderr=b"",
+                returncode=1,
+            )
+        else:
+            return MockCompletedProcess(
+                stdout=b'{"job_id": "foo", "job_name": "bar"}',
+                stderr=b"",
+                returncode=0,
+            )
+    return _get_proc
 
 
 @pytest.fixture(
     params=[
-        (["meta.yaml", "recipe.py"], None),
-        (["meta.yaml", "recipe.py", "requirements.txt"], None),
-        (["meta.yaml", "recipe.py", "requirements.txt"], ValueError),
+        ["meta.yaml", "recipe.py"],
+        ["meta.yaml", "recipe.py", "requirements.txt"],
     ],
-    ids=["no_reqs", "has_reqs", "broken_reqs"],
+    ids=["no_reqs", "has_reqs"],
 )
-def listdir_return_value_with_pip_install_raises(request):
+def listdir_return_value(request):
     return request.param
 
 
@@ -75,7 +100,6 @@ def mock_tempfile_name():
 
 
 @patch("deploy_recipe.os.listdir")
-@patch("deploy_recipe.pip_install")
 @patch("deploy_recipe.subprocess.run")
 @patch("deploy_recipe.requests.get")
 @patch("deploy_recipe.tempfile.NamedTemporaryFile")
@@ -88,12 +112,11 @@ def test_main(
     named_temporary_file: MagicMock,
     requests_get: MagicMock,
     subprocess_run: MagicMock,
-    pip_install: MagicMock,
     listdir: MagicMock,
     env: dict,
     requests_get_returns_json: list,
-    subprocess_return_values: dict,
-    listdir_return_value_with_pip_install_raises: list[str],
+    subprocess_run_side_effect: dict,
+    listdir_return_value: list[str],
     mock_tempfile_name: str,
 ):  
     # mock a context manager, see: https://stackoverflow.com/a/28852060
@@ -102,19 +125,17 @@ def test_main(
     # mock reponse of requests.get call to github api 
     requests_get.return_value.json.return_value = requests_get_returns_json
 
-    # mock result of subprocess call to `pangeo-forge-runner`
-    subprocess_run.return_value.stdout.decode.return_value = subprocess_return_values["stdout"]
-    subprocess_run.return_value.stderr.decode.return_value = subprocess_return_values["stderr"]
-    subprocess_run.return_value.returncode = subprocess_return_values["returncode"]
+    # mock result of subprocess calls
+    subprocess_run.side_effect = subprocess_run_side_effect
 
     # mock listdir call return value, and pip install side effect
-    listdir_return, pip_install_raises = listdir_return_value_with_pip_install_raises
-    listdir.return_value = listdir_return
-    if pip_install_raises:
-        pip_install.side_effect = pip_install_raises()
+    listdir.return_value = listdir_return_value
 
     with patch.dict(os.environ, env):
-        if pip_install_raises:
+        if (
+            "broken-requirements-feedstock" in env["INPUT_PANGEO_FORGE_RUNNER_CONFIG"]
+            and any([path.endswith("requirements.txt") for path in listdir_return_value])
+        ):
             with pytest.raises(ValueError):
                 main()
             # if pip install fails, we should bail early & never invoke `bake`. re: `call_args_list`:
@@ -124,12 +145,22 @@ def test_main(
         else:
             main()
 
-            if any([path.endswith("requirements.txt") for path in listdir_return]):
-                pip_install.assert_any_call("feedstock/requirements.txt", env['CONDA_ENV'])
+            if any([path.endswith("requirements.txt") for path in listdir_return_value]):
+                config = json.loads(env["INPUT_PANGEO_FORGE_RUNNER_CONFIG"])
+                feedstock_subdir = (
+                    config["BaseCommand"]["feedstock-subdir"]
+                    if "BaseCommand" in config
+                    else "feedstock"
+                )
+                expected_cmd = (
+                    f"mamba run -n {env['CONDA_ENV']} "
+                    f"pip install -Ur {feedstock_subdir}/requirements.txt"
+                ).split()
+                subprocess_run.assert_any_call(expected_cmd, capture_output=True)
             else:
                 # if 'requirements.txt' not present, 'pip' is never invoked.
                 # re: `call_args_list`: see link in comment in `if pip_install_raises:` block above.
-                for call in [args[0][0] for args in pip_install.call_args_list]:
+                for call in [args[0][0] for args in subprocess_run.call_args_list]:
                     assert "pip" not in call
 
             listdir.assert_called_once()