feat: Seed commands tests, rework projects internals (#189)

The focus of this diff is to seed example tests using the responses
library for the commands.py file, which currently has no test coverage.
The `projects` and `submit` command, which are among the more utilized
functions are used here as examples.

In the process of adding tests, the projects internals in commands.py
was found to be confusing and made to be more intuitive.
A deprecation warning is attached to the `-i` option in the `projects`
command. Instead of requiring an input which is unused, this turns out
into a flag and renamed to `--names`.

Author: yangchoo

CHANGELOG.rst

@@ -9,6 +9,9 @@ Added
 ~~~~~
 
 - isort for automatic import sorting
+- Example initial tests for `commands` file using `responses` pattern, starting with
+  `submit` and `projects`.
+- Deprecation warning for existing `-i` option for `projects` command.
 - Binder build cache step
 
 Updated
@@ -20,7 +23,16 @@ Fixed
 ~~~~~
 
 - Issue with CodeCov for GitHub action CI
+- `-i` option for `projects` command did not output anything to console when called from
+  cli.
+- Pinned numpy to <=1.19.5 due to an incompatibility issue with numpy 1.20.0 on python 3.7
 
+Updated
+~~~~~~~
+
+- Added new option "--names" to `projects` CLI command. This is meant as a better
+  named and more intuitive replacement for the existing `-i` option.
+- Returned more explicit error statuses for `projects` and `submit` commands.
 
 v9.0.0
 ------

setup.py

@@ -52,7 +52,8 @@ def run_tests(self):
 analysis_deps = [
     "autoprotocol>=7.1,<8",
     "matplotlib>=3,<4",
-    "numpy>=1.14,<2",
+    # incompatibilities with release 1.20.0
+    "numpy>=1.14,<=1.19.5",
     "pandas>=0.23,<1",
     "pillow>=3,<4",
     "plotly>=1.13,<2",

test/cli_test.py

@@ -4,6 +4,7 @@
 
 from click.testing import CliRunner
 from Crypto.PublicKey import RSA
+from transcriptic import commands
 from transcriptic.cli import cli
 
 
@@ -40,7 +41,9 @@ def temp_ssh_key(tmpdir_factory):
 
 @pytest.fixture
 def cli_test_runner(temp_tx_dotfile):
-    runner = CliRunner(env={"TRANSCRIPTIC_CONFIG": str(temp_tx_dotfile)})
+    runner = CliRunner(
+        env={"TRANSCRIPTIC_CONFIG": str(temp_tx_dotfile)}, mix_stderr=False
+    )
     yield runner
 
 
@@ -58,7 +61,7 @@ def test_login_nonexistent_key_path(cli_test_runner):
         ["login", "--rsa-key", "/temp/path/invalid_key.pem"],
         input="\n".join(["email@foo.com", "barpw"]),
     )
-    assert "Invalid value for '--rsa-key'" in result.output
+    assert "Invalid value for '--rsa-key'" in result.stderr
 
 
 def test_login_random_file_for_key(cli_test_runner, tmpdir_factory):
@@ -72,8 +75,9 @@ def test_login_random_file_for_key(cli_test_runner, tmpdir_factory):
     )
     assert (
         "Error loading RSA key: Could not parse the specified RSA Key, "
-        "ensure it is a PRIVATE key in PEM format" in result.output
+        "ensure it is a PRIVATE key in PEM format" in result.stderr
     )
+    assert result.exit_code == 1
 
 
 def test_login_public_key(cli_test_runner, temp_ssh_key):
@@ -84,3 +88,88 @@ def test_login_public_key(cli_test_runner, temp_ssh_key):
         input="\n".join(["email@foo.com", "barpw"]),
     )
     assert "Error connecting to host: This is not a private key" in result.output
+    assert result.exit_code == 1
+
+
+def test_projects_exception(cli_test_runner, monkeypatch):
+    def mocked_exception(api, i, json_flag, names_only):
+        raise RuntimeError("Some runtime error")
+
+    monkeypatch.setattr(commands, "projects", mocked_exception)
+
+    result = cli_test_runner.invoke(
+        cli,
+        ["projects"],
+    )
+    assert result.stderr == (
+        "There was an error listing the projects in your "
+        "organization. Make sure your login details are correct.\n"
+    )
+
+
+def test_projects_names_only(cli_test_runner, monkeypatch):
+    mocked_return = {"p123": "Foo"}
+    monkeypatch.setattr(
+        commands, "projects", lambda api, i, json_flag, names_only: mocked_return
+    )
+
+    result = cli_test_runner.invoke(
+        cli,
+        ["projects", "--names"],
+    )
+    assert result.output == f"{mocked_return}\n"
+
+
+def test_projects_json_flag(cli_test_runner, monkeypatch):
+    mocked_return = [{"archived_at": "some datetime", "id": "p123", "name": "Foo"}]
+    monkeypatch.setattr(
+        commands, "projects", lambda api, i, json_flag, names_only: mocked_return
+    )
+
+    result = cli_test_runner.invoke(
+        cli,
+        ["projects", "--json"],
+    )
+    assert result.output == f"{json.dumps(mocked_return)}\n"
+
+
+def test_projects_default(cli_test_runner, monkeypatch):
+    mocked_return = {"p123": "Foo (archived)"}
+    monkeypatch.setattr(
+        commands, "projects", lambda api, i, json_flag, names_only: mocked_return
+    )
+
+    result = cli_test_runner.invoke(
+        cli,
+        ["projects"],
+    )
+    assert result.output == (
+        "\n"
+        "                                   PROJECTS:\n"
+        "                                   \n"
+        "              PROJECT NAME              |               PROJECT ID               \n"
+        "--------------------------------------------------------------------------------\n"
+        "Foo (archived)                          |                  p123                  \n"
+        "--------------------------------------------------------------------------------\n"
+    )
+
+
+def test_submit_exception_handling(cli_test_runner, monkeypatch):
+    runtime_error = "Some runtime error message"
+
+    def mocked_exception(api, file, project, title, test, pm):
+        raise RuntimeError(runtime_error)
+
+    monkeypatch.setattr(commands, "submit", mocked_exception)
+    result = cli_test_runner.invoke(cli, ["submit", "--project", "some project"])
+    assert result.stderr == f"{runtime_error}\n"
+    assert result.exit_code == 1
+
+
+def test_submit_success(cli_test_runner, monkeypatch):
+    mock_url = "http://mock-api/mock/p123/runs/r123"
+    monkeypatch.setattr(
+        commands, "submit", lambda api, file, project, title, test, pm: mock_url
+    )
+    result = cli_test_runner.invoke(cli, ["submit", "--project", "some project"])
+    assert result.output == f"Run created: {mock_url}\n"

test/commands/__init__.py

@@ -0,0 +1,89 @@
+import json
+
+import pytest
+import responses
+
+from transcriptic import commands
+
+
+class TestProjects:
+    @responses.activate
+    def test_projects_invalid(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json="some verbose error",
+            status=404,
+        )
+
+        with pytest.raises(RuntimeError):
+            commands.projects(test_connection)
+
+    @responses.activate
+    def test_projects_names_only(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json={
+                "projects": [
+                    {"archived_at": "some datetime", "id": "p123", "name": "Foo"}
+                ]
+            },
+        )
+
+        actual = commands.projects(test_connection, names_only=True)
+
+        expected = {"p123": "Foo"}
+        assert actual == expected
+
+    @responses.activate
+    def test_projects_deprecated_i(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json={
+                "projects": [
+                    {"archived_at": "some datetime", "id": "p123", "name": "Foo"}
+                ]
+            },
+        )
+
+        with pytest.warns(FutureWarning):
+            actual = commands.projects(test_connection, i=True)
+
+        expected = {"p123": "Foo"}
+        assert actual == expected
+
+    @responses.activate
+    def test_projects_json_flag(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json={
+                "projects": [
+                    {"archived_at": "some datetime", "id": "p123", "name": "Foo"}
+                ]
+            },
+        )
+
+        actual = commands.projects(test_connection, json_flag=True)
+
+        expected = [{"archived_at": "some datetime", "id": "p123", "name": "Foo"}]
+        assert actual == expected
+
+    @responses.activate
+    def test_projects_default_return(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json={
+                "projects": [
+                    {"archived_at": "some datetime", "id": "p123", "name": "Foo"}
+                ]
+            },
+        )
+
+        actual = commands.projects(test_connection)
+
+        expected = {"p123": "Foo (archived)"}
+        assert actual == expected

test/commands/projects_test.py

@@ -0,0 +1,87 @@
+import json
+
+import pytest
+import responses
+
+from transcriptic import commands
+
+
+class TestSubmit:
+    """
+    Note: Underlying helper functions are tested separately in `TestUtils` class. This
+          uses monkeypatching to mock out those functions.
+    """
+
+    @pytest.fixture
+    def valid_json_file(self, tmpdir):
+        path = tmpdir.mkdir("foo").join("valid-input.json")
+        path.write(json.dumps({"refs": {}, "instructions": []}))
+        yield path
+
+    def test_invalid_pm(self, monkeypatch, test_connection):
+        monkeypatch.setattr(commands, "is_valid_payment_method", lambda api, pm: False)
+
+        with pytest.raises(RuntimeError) as error:
+            commands.submit(test_connection, "some file", "some project", pm="invalid")
+
+        assert f"{error.value}" == (
+            "Payment method is invalid. Please specify a payment "
+            "method from `transcriptic payments` or not specify the "
+            "`--payment` flag to use the default payment method."
+        )
+
+    def test_invalid_project(self, monkeypatch, test_connection):
+        monkeypatch.setattr(commands, "get_project_id", lambda api, project: False)
+
+        with pytest.raises(RuntimeError) as error:
+            commands.submit(test_connection, "some file", "invalid_project")
+
+        assert f"{error.value}" == "Invalid project invalid_project specified"
+
+    def test_invalid_file(self, monkeypatch, test_connection, tmpdir):
+        path = tmpdir.mkdir("foo").join("invalid-input.txt")
+        path.write("this is not json")
+
+        monkeypatch.setattr(commands, "get_project_id", lambda api, project: "p123")
+
+        with pytest.raises(RuntimeError) as error:
+            commands.submit(test_connection, path, "project name")
+
+        assert (
+            f"{error.value}"
+            == "Error: Could not submit since your manifest.json file is improperly "
+            "formatted."
+        )
+
+    @responses.activate
+    def test_valid_submission(self, monkeypatch, test_connection, valid_json_file):
+        monkeypatch.setattr(commands, "get_project_id", lambda api, project: "p123")
+
+        responses.add(
+            responses.POST,
+            test_connection.get_route("submit_run", project_id="p123"),
+            json={"id": "r123"},
+        )
+
+        actual = commands.submit(test_connection, valid_json_file, "project name")
+
+        expected = "http://mock-api/mock/p123/runs/r123"
+        assert actual == expected
+
+    @responses.activate
+    def test_submit_exception_handling(
+        self, monkeypatch, test_connection, valid_json_file
+    ):
+        monkeypatch.setattr(commands, "get_project_id", lambda api, project: "p123")
+
+        responses.add(
+            responses.POST,
+            test_connection.get_route("submit_run", project_id="p123"),
+            json="some verbose error",
+            status=404,
+        )
+
+        with pytest.raises(RuntimeError) as error:
+            commands.submit(test_connection, valid_json_file, "project name")
+
+        assert "Error: Couldn't create run (404)" in f"{error.value}"

test/commands/submit_test.py

@@ -0,0 +1,40 @@
+import responses
+
+from transcriptic import commands
+
+
+class TestUtils:
+    @responses.activate
+    def test_valid_payment_method_true(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_payment_methods"),
+            json=[{"id": "someId", "is_valid": True}],
+        )
+        assert commands.is_valid_payment_method(test_connection, "someId")
+
+    @responses.activate
+    def test_valid_payment_method_false(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_payment_methods"),
+            json=[{"id": "someId", "is_valid": True}],
+        )
+        assert not commands.is_valid_payment_method(test_connection, "invalidId")
+
+    @responses.activate
+    def test_get_project_id(self, test_connection):
+        responses.add(
+            responses.GET,
+            test_connection.get_route("get_projects"),
+            json={
+                "projects": [
+                    {"archived_at": "some datetime", "id": "p123", "name": "Foo"}
+                ]
+            },
+        )
+
+        actual = commands.get_project_id(test_connection, "Foo")
+
+        expected = "p123"
+        assert actual == expected