diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4d601142..a85179a6 100644 --- a/CHANGELOG.rst +++ b/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 ------ diff --git a/setup.py b/setup.py index 7ee7739c..5d3f9122 100644 --- a/setup.py +++ b/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", diff --git a/test/cli_test.py b/test/cli_test.py index 2aae6d5e..00adfb77 100644 --- a/test/cli_test.py +++ b/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" diff --git a/test/commands/__init__.py b/test/commands/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/test/commands/projects_test.py b/test/commands/projects_test.py new file mode 100644 index 00000000..04788b16 --- /dev/null +++ b/test/commands/projects_test.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 diff --git a/test/commands/submit_test.py b/test/commands/submit_test.py new file mode 100644 index 00000000..a7e32216 --- /dev/null +++ b/test/commands/submit_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}" diff --git a/test/commands/utils_test.py b/test/commands/utils_test.py new file mode 100644 index 00000000..830de149 --- /dev/null +++ b/test/commands/utils_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 diff --git a/test/conftest.py b/test/conftest.py index 9fd1f5c9..cab9073e 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -10,6 +10,7 @@ sys.path.append(os.path.join(os.path.dirname(__file__), "helpers")) +# TODO: Migrate tests utilizing this to use the `TestConnection` pattern instead. @pytest.fixture() def test_api(monkeypatch): from transcriptic.config import Connection @@ -43,7 +44,10 @@ def json_db(): class ResponseDB: - """Contains dictionary of MockResponse used for testing purposes""" + """ + Contains dictionary of MockResponse used for testing purposes + TODO: Simplify this by using `@responses.activate` pattern + """ def __init__(self): self.mock_responses = {} @@ -61,3 +65,12 @@ def reset(self): @pytest.fixture(scope="module") def response_db(): return ResponseDB() + + +@pytest.fixture(scope="module") +def test_connection(): + from transcriptic.config import Connection + + return Connection( + email="mock@api.com", organization_id="mock", api_root="http://mock-api" + ) diff --git a/transcriptic/cli.py b/transcriptic/cli.py index c5b75f11..da24d50d 100755 --- a/transcriptic/cli.py +++ b/transcriptic/cli.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 - +import json import os +import sys import click import requests @@ -204,7 +205,12 @@ def cli(ctx, api_root, email, token, organization, config): def submit_cmd(ctx, file, project, title=None, test=None, pm=None): """Submit your run to the project specified.""" api = ctx.obj.api - commands.submit(api, file, project, title=title, test=test, pm=pm) + try: + run_url = commands.submit(api, file, project, title=title, test=test, pm=pm) + click.echo(f"Run created: {run_url}") + except RuntimeError as err: + click.echo(f"{err}", err=True) + sys.exit(1) @cli.command("build-release", cls=FeatureCommand, feature="can_upload_packages") @@ -313,12 +319,36 @@ def generate_protocol_cmd(ctx, name): @cli.command("projects") @click.pass_context -@click.option("-i") +@click.option("-i", help="DEPRECATED option. Use `--names` instead.") @click.option("--json", "json_flag", help="print JSON response", is_flag=True) -def projects_cmd(ctx, i, json_flag): +@click.option( + "--names", + "names_only", + help="returns a mapping of `project_id`: `project_name`", + is_flag=True, +) +def projects_cmd(ctx, i, json_flag, names_only): """List the projects in your organization""" api = ctx.obj.api - commands.projects(api, i, json_flag) + try: + response = commands.projects(api, i, json_flag, names_only) + if i or names_only: + click.echo(response) + elif json_flag: + click.echo(json.dumps(response)) + else: + click.echo("\n{:^80}".format("PROJECTS:\n")) + click.echo(f"{'PROJECT NAME':^40}" + "|" + f"{'PROJECT ID':^40}") + click.echo(f"{'':-^80}") + for proj_id, name in list(response.items()): + click.echo(f"{name:<40}" + "|" + f"{proj_id:^40}") + click.echo(f"{'':-^80}") + except RuntimeError: + click.echo( + "There was an error listing the projects in your " + "organization. Make sure your login details are correct.", + err=True, + ) @cli.command("runs") diff --git a/transcriptic/commands.py b/transcriptic/commands.py index a9311fcf..6fa3f2de 100644 --- a/transcriptic/commands.py +++ b/transcriptic/commands.py @@ -1,10 +1,18 @@ #!/usr/bin/env python3 +""" +Contains abstracted functions which is primarily used by the CLI. However, they can +be separately imported and used in other contexts. + +There is a mix of functions which directly call `click.echo` vs returning responses to +the caller (e.g. CLI). We should move towards the latter pattern. +""" import json import locale import os import sys import time +import warnings import zipfile from collections import OrderedDict @@ -17,45 +25,69 @@ from jinja2 import Environment, PackageLoader from transcriptic import routes from transcriptic.auth import StrateosSign -from transcriptic.config import Connection +from transcriptic.config import AnalysisException, Connection from transcriptic.english import AutoprotocolParser from transcriptic.util import ascii_encode, flatmap, iter_json, makedirs -def submit(api, file, project, title=None, test=None, pm=None): - """Submit your run to the project specified.""" +def submit( + api: Connection, + file: str, + project: str, + title: str = None, + test: bool = None, + pm: str = None, +): + """ + Submit your run to the project specified. + If successful, returns the formatted url link to the created run. + + Parameters + ---------- + api: Connection + API context used for making base calls + file: str + Name of file to read from. Use `-` if reading from standard input. + project: str + `ProjectId` to submit this json to. + title: str, optional + If specified, Title of the created run. + test: bool, optional + If true, submit as a test run. + pm: str, optional + If specified, `PaymentId` to be used. + """ if pm is not None and not is_valid_payment_method(api, pm): - print_stderr( + raise RuntimeError( "Payment method is invalid. Please specify a payment " "method from `transcriptic payments` or not specify the " "`--payment` flag to use the default payment method." ) - return - project = get_project_id(api, project) - if not project: - return + valid_project_id = get_project_id(api, project) + if not valid_project_id: + raise RuntimeError(f"Invalid project {project} specified") with click.open_file(file, "r") as f: try: protocol = json.loads(f.read()) except ValueError: - click.echo( + raise RuntimeError( "Error: Could not submit since your manifest.json " "file is improperly formatted." ) - return try: req_json = api.submit_run( protocol, - project_id=project, + project_id=valid_project_id, title=title, test_mode=test, payment_method_id=pm, ) run_id = req_json["id"] - click.echo("Run created: %s" % api.url("%s/runs/%s" % (project, run_id))) - except Exception as err: - click.echo("\n" + str(err)) + formatted_url = api.url(f"{valid_project_id}/runs/{run_id}") + return formatted_url + except AnalysisException as e: + raise RuntimeError(e.message) def release(api, name=None, package=None): @@ -350,32 +382,50 @@ def upload_dataset(api, file_path, title, run_id, tool, version): click.echo("An unexpected response was returned from the server. ") -def projects(api, i, json_flag): - """List the projects in your organization""" - try: - projects = api.projects() - proj_id_names = {} - all_proj = {} - for proj in projects: - status = " (archived)" if proj["archived_at"] else "" - proj_id_names[proj["id"]] = proj["name"] - all_proj[proj["id"]] = proj["name"] + status - if i: - return proj_id_names - elif json_flag: - return click.echo(json.dumps(projects)) - else: - click.echo("\n{:^80}".format("PROJECTS:\n")) - click.echo(f"{'PROJECT NAME':^40}" + "|" + f"{'PROJECT ID':^40}") - click.echo(f"{'':-^80}") - for proj_id, name in list(all_proj.items()): - click.echo(f"{name:<40}" + "|" + f"{proj_id:^40}") - click.echo(f"{'':-^80}") - except RuntimeError: - click.echo( - "There was an error listing the projects in your " - "organization. Make sure your login details are correct." +def projects( + api: Connection, + i: any = None, + json_flag: bool = False, + names_only: bool = False, +): + """ + List the projects in your organization. + + When no options are specified, returns a summarized format. + + Parameters + ---------- + api: Connection + API context used for making base calls + i: any, optional + DEPRECATED option. See `names_only`. + json_flag: bool, optional + Returns the full response which is json formatted. + names_only: bool, optional + Returns a `project_id: project_name` mapping. + """ + if i: + warnings.warn( + "`i` will be deprecated in the future. Please use `names_only` instead.", + FutureWarning, ) + names_only = True + + response = api.projects() + + proj_id_names = {} + all_proj = {} + for proj in response: + status = " (archived)" if proj["archived_at"] else "" + proj_id_names[proj["id"]] = proj["name"] + all_proj[proj["id"]] = proj["name"] + status + + if names_only: + return proj_id_names + elif json_flag: + return response + else: + return all_proj def runs(api, project_name, json_flag): @@ -1016,7 +1066,8 @@ def login(api, config, api_root=None, analytics=True, rsa_key=None): except Exception: click.echo( f"Error loading RSA key. Please check that the file " - f"{rsa_key} is accessible" + f"{rsa_key} is accessible", + err=True, ) sys.exit(1) @@ -1025,7 +1076,7 @@ def login(api, config, api_root=None, analytics=True, rsa_key=None): try: rsa_auth = StrateosSign("foo@bar.com", rsa_secret, api_root) except Exception as e: - click.echo(f"Error loading RSA key: {e}") + click.echo(f"Error loading RSA key: {e}", err=True) sys.exit(1) email = click.prompt("Email") @@ -1290,11 +1341,11 @@ def parse_valid_org(indx): def get_project_id(api, name): - projs = projects(api, True, True) - if name in projs: + project_id_name_mapping = projects(api, names_only=True) + if name in project_id_name_mapping: return name else: - project_ids = [k for k, v in projs.items() if v == name] + project_ids = [k for k, v in project_id_name_mapping.items() if v == name] if not project_ids: click.echo(f"The project '{name}' was not found in your organization.") return @@ -1307,10 +1358,10 @@ def get_project_id(api, name): def get_project_name(api, id): - projs = projects(api, True, True) - name = projs.get(id) + project_id_name_mapping = projects(api, names_only=True) + name = project_id_name_mapping.get(id) if not name: - name = id if id in projs.values() else None + name = id if id in project_id_name_mapping.values() else None if not name: click.echo(f"The project '{name}' was not found in your organization.") return diff --git a/transcriptic/config.py b/transcriptic/config.py index b8f9aae2..9fd1bb69 100644 --- a/transcriptic/config.py +++ b/transcriptic/config.py @@ -424,7 +424,7 @@ def projects(self): route = self.get_route("get_projects") err_default = ( "There was an error listing the projects in your " - "organization. Make sure your login details are correct." + "organization. Make sure your login details are correct." ) return self.get( route,