From f77bd5e412cce01e785b953285379f78926ccf2d Mon Sep 17 00:00:00 2001 From: yangchoo Date: Tue, 19 Jan 2021 00:32:48 -0800 Subject: [PATCH 1/6] feat: Seed commands tests, rework projects internals 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`. --- CHANGELOG.rst | 11 ++ test/commands_test.py | 211 +++++++++++++++++++++++++++++++++++++++ test/conftest.py | 15 ++- transcriptic/cli.py | 12 ++- transcriptic/commands.py | 103 +++++++++++++++---- transcriptic/config.py | 2 +- 6 files changed, 327 insertions(+), 27 deletions(-) create mode 100644 test/commands_test.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 558aa256..f6e23e2b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,12 +9,23 @@ 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. Fixed ~~~~~ - Issue with CodeCov for GitHub action CI +- `-i` option for `projects` command did not output anything to console when called from + cli. +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/test/commands_test.py b/test/commands_test.py new file mode 100644 index 00000000..7d50697f --- /dev/null +++ b/test/commands_test.py @@ -0,0 +1,211 @@ +import json + +import pytest +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") + + +class TestProjects: + @responses.activate + def test_projects_invalid(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json="some verbose error", + status=404, + ) + + commands.projects(test_connection) + + captured = capsys.readouterr() + assert captured.err == ( + "There was an error listing the projects in your " + "organization. Make sure your login details are correct.\n" + ) + + @responses.activate + def test_projects_names_only(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json={ + "projects": [ + {"archived_at": "some datetime", "id": "p123", "name": "Foo"} + ] + }, + ) + + ret = commands.projects(test_connection, names_only=True) + + expected = {"p123": "Foo"} + assert ret == expected + captured = capsys.readouterr() + assert captured.out == f"{expected}\n" + + @responses.activate + def test_projects_deprecated_i(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json={ + "projects": [ + {"archived_at": "some datetime", "id": "p123", "name": "Foo"} + ] + }, + ) + + with pytest.warns(FutureWarning): + ret = commands.projects(test_connection, i=True) + + expected = {"p123": "Foo"} + assert ret == expected + captured = capsys.readouterr() + assert captured.out == f"{expected}\n" + + @responses.activate + def test_projects_json_flag(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json={ + "projects": [ + {"archived_at": "some datetime", "id": "p123", "name": "Foo"} + ] + }, + ) + + ret = commands.projects(test_connection, json_flag=True) + + expected = [{"archived_at": "some datetime", "id": "p123", "name": "Foo"}] + assert ret == expected + captured = capsys.readouterr() + assert captured.out == f"{json.dumps(expected)}\n" + + @responses.activate + def test_projects_default_return(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json={ + "projects": [ + {"archived_at": "some datetime", "id": "p123", "name": "Foo"} + ] + }, + ) + + commands.projects(test_connection) + + captured = capsys.readouterr() + assert captured.out == ( + "\n" + " PROJECTS:\n" + " \n" + " PROJECT NAME | PROJECT ID \n" + "--------------------------------------------------------------------------------\n" + "Foo (archived) | p123 \n" + "--------------------------------------------------------------------------------\n" + ) + + +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, capsys): + monkeypatch.setattr(commands, "is_valid_payment_method", lambda api, pm: False) + + commands.submit(test_connection, "some file", "some project", pm="invalid") + + captured = capsys.readouterr() + assert captured.err == ( + "Payment method is invalid. Please specify a payment " + "method from `transcriptic payments` or not specify the " + "`--payment` flag to use the default payment method.\n" + ) + + def test_invalid_project(self, monkeypatch, test_connection, capsys): + monkeypatch.setattr(commands, "get_project_id", lambda api, project: False) + + commands.submit(test_connection, "some file", "invalid_project") + + captured = capsys.readouterr() + assert captured.err == "Invalid project invalid_project specified\n" + + def test_invalid_file(self, monkeypatch, test_connection, capsys, 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") + + commands.submit(test_connection, path, "project name") + + captured = capsys.readouterr() + assert ( + captured.err + == "Error: Could not submit since your manifest.json file is improperly " + "formatted.\n" + ) + + @responses.activate + def test_valid_submission( + self, monkeypatch, test_connection, capsys, 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"}, + ) + + commands.submit(test_connection, valid_json_file, "project name") + + captured = capsys.readouterr() + assert captured.out == "Run created: http://mock-api/mock/p123/runs/r123\n" + + @responses.activate + def test_submit_exception_handling( + self, monkeypatch, test_connection, capsys, 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, + ) + + commands.submit(test_connection, valid_json_file, "project name") + + captured = capsys.readouterr() + assert "Error: Couldn't create run (404)" in captured.err 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..6e1c9303 100755 --- a/transcriptic/cli.py +++ b/transcriptic/cli.py @@ -313,12 +313,18 @@ 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) + commands.projects(api, i, json_flag, names_only) @cli.command("runs") diff --git a/transcriptic/commands.py b/transcriptic/commands.py index 3f5839ad..c63025e7 100644 --- a/transcriptic/commands.py +++ b/transcriptic/commands.py @@ -1,10 +1,15 @@ #!/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. +""" import json import locale import os import sys import time +import warnings import zipfile from collections import OrderedDict @@ -22,8 +27,32 @@ 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. + + 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( "Payment method is invalid. Please specify a payment " @@ -31,14 +60,15 @@ def submit(api, file, project, title=None, test=None, pm=None): "`--payment` flag to use the default payment method." ) return - project = get_project_id(api, project) - if not project: + valid_project_id = get_project_id(api, project) + if not valid_project_id: + print_stderr(f"Invalid project {project} specified") return with click.open_file(file, "r") as f: try: protocol = json.loads(f.read()) except ValueError: - click.echo( + print_stderr( "Error: Could not submit since your manifest.json " "file is improperly formatted." ) @@ -47,15 +77,16 @@ def submit(api, file, project, title=None, test=None, pm=None): 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))) + formatted_url = api.url(f"{valid_project_id}/runs/{run_id}") + click.echo(f"Run created: {formatted_url}") except Exception as err: - click.echo("\n" + str(err)) + print_stderr(str(err)) def release(api, name=None, package=None): @@ -350,20 +381,48 @@ 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""" +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, outputs a console-optimized format. + + Parameters + ---------- + api: Connection + API context used for making base calls + i: any, optional + DEPRECATED option. See `names_only`. + json_flag: bool, optional + Outputs to console and returns the full response which is json formatted. + names_only: bool, optional + Outputs to console and 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 try: - projects = api.projects() + response = api.projects() proj_id_names = {} all_proj = {} - for proj in projects: + 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 i: + if names_only: + click.echo(proj_id_names) return proj_id_names elif json_flag: - return click.echo(json.dumps(projects)) + click.echo(json.dumps(response)) + return response else: click.echo("\n{:^80}".format("PROJECTS:\n")) click.echo(f"{'PROJECT NAME':^40}" + "|" + f"{'PROJECT ID':^40}") @@ -372,9 +431,9 @@ def projects(api, i, json_flag): click.echo(f"{name:<40}" + "|" + f"{proj_id:^40}") click.echo(f"{'':-^80}") except RuntimeError: - click.echo( + print_stderr( "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." ) @@ -1290,11 +1349,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 +1366,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, From f47097c277ac603f93a4c28f0e3743051f675b17 Mon Sep 17 00:00:00 2001 From: yangchoo Date: Wed, 20 Jan 2021 19:51:09 -0800 Subject: [PATCH 2/6] address comments: rename, breakout tests --- test/commands/__init__.py | 0 test/commands/projects_test.py | 108 +++++++++++++++++ test/commands/submit_test.py | 89 ++++++++++++++ test/commands/utils_test.py | 23 ++++ test/commands_test.py | 211 --------------------------------- 5 files changed, 220 insertions(+), 211 deletions(-) create mode 100644 test/commands/__init__.py create mode 100644 test/commands/projects_test.py create mode 100644 test/commands/submit_test.py create mode 100644 test/commands/utils_test.py delete mode 100644 test/commands_test.py 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..c4ed9014 --- /dev/null +++ b/test/commands/projects_test.py @@ -0,0 +1,108 @@ +import json + +import pytest +import responses + +from transcriptic import commands + + +class TestProjects: + @responses.activate + def test_projects_invalid(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json="some verbose error", + status=404, + ) + + commands.projects(test_connection) + + captured = capsys.readouterr() + assert captured.err == ( + "There was an error listing the projects in your " + "organization. Make sure your login details are correct.\n" + ) + + @responses.activate + def test_projects_names_only(self, test_connection, capsys): + 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 + captured = capsys.readouterr() + assert captured.out == f"{expected}\n" + + @responses.activate + def test_projects_deprecated_i(self, test_connection, capsys): + 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 + captured = capsys.readouterr() + assert captured.out == f"{expected}\n" + + @responses.activate + def test_projects_json_flag(self, test_connection, capsys): + 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 + captured = capsys.readouterr() + assert captured.out == f"{json.dumps(expected)}\n" + + @responses.activate + def test_projects_default_return(self, test_connection, capsys): + responses.add( + responses.GET, + test_connection.get_route("get_projects"), + json={ + "projects": [ + {"archived_at": "some datetime", "id": "p123", "name": "Foo"} + ] + }, + ) + + commands.projects(test_connection) + + captured = capsys.readouterr() + assert captured.out == ( + "\n" + " PROJECTS:\n" + " \n" + " PROJECT NAME | PROJECT ID \n" + "--------------------------------------------------------------------------------\n" + "Foo (archived) | p123 \n" + "--------------------------------------------------------------------------------\n" + ) diff --git a/test/commands/submit_test.py b/test/commands/submit_test.py new file mode 100644 index 00000000..b424e643 --- /dev/null +++ b/test/commands/submit_test.py @@ -0,0 +1,89 @@ +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, capsys): + monkeypatch.setattr(commands, "is_valid_payment_method", lambda api, pm: False) + + commands.submit(test_connection, "some file", "some project", pm="invalid") + + captured = capsys.readouterr() + assert captured.err == ( + "Payment method is invalid. Please specify a payment " + "method from `transcriptic payments` or not specify the " + "`--payment` flag to use the default payment method.\n" + ) + + def test_invalid_project(self, monkeypatch, test_connection, capsys): + monkeypatch.setattr(commands, "get_project_id", lambda api, project: False) + + commands.submit(test_connection, "some file", "invalid_project") + + captured = capsys.readouterr() + assert captured.err == "Invalid project invalid_project specified\n" + + def test_invalid_file(self, monkeypatch, test_connection, capsys, 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") + + commands.submit(test_connection, path, "project name") + + captured = capsys.readouterr() + assert ( + captured.err + == "Error: Could not submit since your manifest.json file is improperly " + "formatted.\n" + ) + + @responses.activate + def test_valid_submission( + self, monkeypatch, test_connection, capsys, 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"}, + ) + + commands.submit(test_connection, valid_json_file, "project name") + + captured = capsys.readouterr() + assert captured.out == "Run created: http://mock-api/mock/p123/runs/r123\n" + + @responses.activate + def test_submit_exception_handling( + self, monkeypatch, test_connection, capsys, 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, + ) + + commands.submit(test_connection, valid_json_file, "project name") + + captured = capsys.readouterr() + assert "Error: Couldn't create run (404)" in captured.err diff --git a/test/commands/utils_test.py b/test/commands/utils_test.py new file mode 100644 index 00000000..b753aa49 --- /dev/null +++ b/test/commands/utils_test.py @@ -0,0 +1,23 @@ +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") diff --git a/test/commands_test.py b/test/commands_test.py deleted file mode 100644 index 7d50697f..00000000 --- a/test/commands_test.py +++ /dev/null @@ -1,211 +0,0 @@ -import json - -import pytest -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") - - -class TestProjects: - @responses.activate - def test_projects_invalid(self, test_connection, capsys): - responses.add( - responses.GET, - test_connection.get_route("get_projects"), - json="some verbose error", - status=404, - ) - - commands.projects(test_connection) - - captured = capsys.readouterr() - assert captured.err == ( - "There was an error listing the projects in your " - "organization. Make sure your login details are correct.\n" - ) - - @responses.activate - def test_projects_names_only(self, test_connection, capsys): - responses.add( - responses.GET, - test_connection.get_route("get_projects"), - json={ - "projects": [ - {"archived_at": "some datetime", "id": "p123", "name": "Foo"} - ] - }, - ) - - ret = commands.projects(test_connection, names_only=True) - - expected = {"p123": "Foo"} - assert ret == expected - captured = capsys.readouterr() - assert captured.out == f"{expected}\n" - - @responses.activate - def test_projects_deprecated_i(self, test_connection, capsys): - responses.add( - responses.GET, - test_connection.get_route("get_projects"), - json={ - "projects": [ - {"archived_at": "some datetime", "id": "p123", "name": "Foo"} - ] - }, - ) - - with pytest.warns(FutureWarning): - ret = commands.projects(test_connection, i=True) - - expected = {"p123": "Foo"} - assert ret == expected - captured = capsys.readouterr() - assert captured.out == f"{expected}\n" - - @responses.activate - def test_projects_json_flag(self, test_connection, capsys): - responses.add( - responses.GET, - test_connection.get_route("get_projects"), - json={ - "projects": [ - {"archived_at": "some datetime", "id": "p123", "name": "Foo"} - ] - }, - ) - - ret = commands.projects(test_connection, json_flag=True) - - expected = [{"archived_at": "some datetime", "id": "p123", "name": "Foo"}] - assert ret == expected - captured = capsys.readouterr() - assert captured.out == f"{json.dumps(expected)}\n" - - @responses.activate - def test_projects_default_return(self, test_connection, capsys): - responses.add( - responses.GET, - test_connection.get_route("get_projects"), - json={ - "projects": [ - {"archived_at": "some datetime", "id": "p123", "name": "Foo"} - ] - }, - ) - - commands.projects(test_connection) - - captured = capsys.readouterr() - assert captured.out == ( - "\n" - " PROJECTS:\n" - " \n" - " PROJECT NAME | PROJECT ID \n" - "--------------------------------------------------------------------------------\n" - "Foo (archived) | p123 \n" - "--------------------------------------------------------------------------------\n" - ) - - -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, capsys): - monkeypatch.setattr(commands, "is_valid_payment_method", lambda api, pm: False) - - commands.submit(test_connection, "some file", "some project", pm="invalid") - - captured = capsys.readouterr() - assert captured.err == ( - "Payment method is invalid. Please specify a payment " - "method from `transcriptic payments` or not specify the " - "`--payment` flag to use the default payment method.\n" - ) - - def test_invalid_project(self, monkeypatch, test_connection, capsys): - monkeypatch.setattr(commands, "get_project_id", lambda api, project: False) - - commands.submit(test_connection, "some file", "invalid_project") - - captured = capsys.readouterr() - assert captured.err == "Invalid project invalid_project specified\n" - - def test_invalid_file(self, monkeypatch, test_connection, capsys, 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") - - commands.submit(test_connection, path, "project name") - - captured = capsys.readouterr() - assert ( - captured.err - == "Error: Could not submit since your manifest.json file is improperly " - "formatted.\n" - ) - - @responses.activate - def test_valid_submission( - self, monkeypatch, test_connection, capsys, 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"}, - ) - - commands.submit(test_connection, valid_json_file, "project name") - - captured = capsys.readouterr() - assert captured.out == "Run created: http://mock-api/mock/p123/runs/r123\n" - - @responses.activate - def test_submit_exception_handling( - self, monkeypatch, test_connection, capsys, 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, - ) - - commands.submit(test_connection, valid_json_file, "project name") - - captured = capsys.readouterr() - assert "Error: Couldn't create run (404)" in captured.err From 6d94c7fecba6dfeeb7b1a18375182f5ede4c8522 Mon Sep 17 00:00:00 2001 From: yangchoo Date: Thu, 21 Jan 2021 08:14:09 -0800 Subject: [PATCH 3/6] refactor to separate responsibilities of commands/cli --- test/cli_test.py | 74 ++++++++++++++++++++++++++++++++-- test/commands/projects_test.py | 41 +++++-------------- test/commands/utils_test.py | 17 ++++++++ transcriptic/cli.py | 22 +++++++++- transcriptic/commands.py | 56 ++++++++++++------------- 5 files changed, 144 insertions(+), 66 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index 2aae6d5e..c435a838 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,67 @@ 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" + ) diff --git a/test/commands/projects_test.py b/test/commands/projects_test.py index c4ed9014..04788b16 100644 --- a/test/commands/projects_test.py +++ b/test/commands/projects_test.py @@ -8,7 +8,7 @@ class TestProjects: @responses.activate - def test_projects_invalid(self, test_connection, capsys): + def test_projects_invalid(self, test_connection): responses.add( responses.GET, test_connection.get_route("get_projects"), @@ -16,16 +16,11 @@ def test_projects_invalid(self, test_connection, capsys): status=404, ) - commands.projects(test_connection) - - captured = capsys.readouterr() - assert captured.err == ( - "There was an error listing the projects in your " - "organization. Make sure your login details are correct.\n" - ) + with pytest.raises(RuntimeError): + commands.projects(test_connection) @responses.activate - def test_projects_names_only(self, test_connection, capsys): + def test_projects_names_only(self, test_connection): responses.add( responses.GET, test_connection.get_route("get_projects"), @@ -40,11 +35,9 @@ def test_projects_names_only(self, test_connection, capsys): expected = {"p123": "Foo"} assert actual == expected - captured = capsys.readouterr() - assert captured.out == f"{expected}\n" @responses.activate - def test_projects_deprecated_i(self, test_connection, capsys): + def test_projects_deprecated_i(self, test_connection): responses.add( responses.GET, test_connection.get_route("get_projects"), @@ -60,11 +53,9 @@ def test_projects_deprecated_i(self, test_connection, capsys): expected = {"p123": "Foo"} assert actual == expected - captured = capsys.readouterr() - assert captured.out == f"{expected}\n" @responses.activate - def test_projects_json_flag(self, test_connection, capsys): + def test_projects_json_flag(self, test_connection): responses.add( responses.GET, test_connection.get_route("get_projects"), @@ -79,11 +70,9 @@ def test_projects_json_flag(self, test_connection, capsys): expected = [{"archived_at": "some datetime", "id": "p123", "name": "Foo"}] assert actual == expected - captured = capsys.readouterr() - assert captured.out == f"{json.dumps(expected)}\n" @responses.activate - def test_projects_default_return(self, test_connection, capsys): + def test_projects_default_return(self, test_connection): responses.add( responses.GET, test_connection.get_route("get_projects"), @@ -94,15 +83,7 @@ def test_projects_default_return(self, test_connection, capsys): }, ) - commands.projects(test_connection) - - captured = capsys.readouterr() - assert captured.out == ( - "\n" - " PROJECTS:\n" - " \n" - " PROJECT NAME | PROJECT ID \n" - "--------------------------------------------------------------------------------\n" - "Foo (archived) | p123 \n" - "--------------------------------------------------------------------------------\n" - ) + actual = commands.projects(test_connection) + + expected = {"p123": "Foo (archived)"} + assert actual == expected diff --git a/test/commands/utils_test.py b/test/commands/utils_test.py index b753aa49..830de149 100644 --- a/test/commands/utils_test.py +++ b/test/commands/utils_test.py @@ -21,3 +21,20 @@ def test_valid_payment_method_false(self, test_connection): 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/transcriptic/cli.py b/transcriptic/cli.py index 6e1c9303..469633a2 100755 --- a/transcriptic/cli.py +++ b/transcriptic/cli.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 - +import json import os import click @@ -324,7 +324,25 @@ def generate_protocol_cmd(ctx, name): 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, names_only) + 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 c63025e7..03c24273 100644 --- a/transcriptic/commands.py +++ b/transcriptic/commands.py @@ -2,6 +2,9 @@ """ 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 @@ -390,7 +393,7 @@ def projects( """ List the projects in your organization. - When no options are specified, outputs a console-optimized format. + When no options are specified, returns a summarized format. Parameters ---------- @@ -399,9 +402,9 @@ def projects( i: any, optional DEPRECATED option. See `names_only`. json_flag: bool, optional - Outputs to console and returns the full response which is json formatted. + Returns the full response which is json formatted. names_only: bool, optional - Outputs to console and returns a `project_id: project_name` mapping. + Returns a `project_id: project_name` mapping. """ if i: warnings.warn( @@ -409,32 +412,22 @@ def projects( FutureWarning, ) names_only = True - try: - 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: - click.echo(proj_id_names) - return proj_id_names - elif json_flag: - click.echo(json.dumps(response)) - return 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(all_proj.items()): - click.echo(f"{name:<40}" + "|" + f"{proj_id:^40}") - click.echo(f"{'':-^80}") - except RuntimeError: - print_stderr( - "There was an error listing the projects in your " - "organization. Make sure your login details are correct." - ) + + 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): @@ -1075,7 +1068,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) @@ -1084,7 +1078,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") From 75665d0ed9fbea19915aeea469ca2eb88bf414cb Mon Sep 17 00:00:00 2001 From: yangchoo Date: Thu, 21 Jan 2021 22:36:56 -0800 Subject: [PATCH 4/6] switch submit tests to new format --- test/cli_test.py | 21 ++++++++++++++++ test/commands/submit_test.py | 46 +++++++++++++++++------------------- transcriptic/cli.py | 8 ++++++- transcriptic/commands.py | 18 +++++++------- 4 files changed, 58 insertions(+), 35 deletions(-) diff --git a/test/cli_test.py b/test/cli_test.py index c435a838..00adfb77 100644 --- a/test/cli_test.py +++ b/test/cli_test.py @@ -152,3 +152,24 @@ def test_projects_default(cli_test_runner, monkeypatch): "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/submit_test.py b/test/commands/submit_test.py index b424e643..a7e32216 100644 --- a/test/commands/submit_test.py +++ b/test/commands/submit_test.py @@ -18,45 +18,43 @@ def valid_json_file(self, tmpdir): path.write(json.dumps({"refs": {}, "instructions": []})) yield path - def test_invalid_pm(self, monkeypatch, test_connection, capsys): + def test_invalid_pm(self, monkeypatch, test_connection): monkeypatch.setattr(commands, "is_valid_payment_method", lambda api, pm: False) - commands.submit(test_connection, "some file", "some project", pm="invalid") + with pytest.raises(RuntimeError) as error: + commands.submit(test_connection, "some file", "some project", pm="invalid") - captured = capsys.readouterr() - assert captured.err == ( + 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.\n" + "`--payment` flag to use the default payment method." ) - def test_invalid_project(self, monkeypatch, test_connection, capsys): + def test_invalid_project(self, monkeypatch, test_connection): monkeypatch.setattr(commands, "get_project_id", lambda api, project: False) - commands.submit(test_connection, "some file", "invalid_project") + with pytest.raises(RuntimeError) as error: + commands.submit(test_connection, "some file", "invalid_project") - captured = capsys.readouterr() - assert captured.err == "Invalid project invalid_project specified\n" + assert f"{error.value}" == "Invalid project invalid_project specified" - def test_invalid_file(self, monkeypatch, test_connection, capsys, tmpdir): + 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") - commands.submit(test_connection, path, "project name") + with pytest.raises(RuntimeError) as error: + commands.submit(test_connection, path, "project name") - captured = capsys.readouterr() assert ( - captured.err + f"{error.value}" == "Error: Could not submit since your manifest.json file is improperly " - "formatted.\n" + "formatted." ) @responses.activate - def test_valid_submission( - self, monkeypatch, test_connection, capsys, valid_json_file - ): + def test_valid_submission(self, monkeypatch, test_connection, valid_json_file): monkeypatch.setattr(commands, "get_project_id", lambda api, project: "p123") responses.add( @@ -65,14 +63,14 @@ def test_valid_submission( json={"id": "r123"}, ) - commands.submit(test_connection, valid_json_file, "project name") + actual = commands.submit(test_connection, valid_json_file, "project name") - captured = capsys.readouterr() - assert captured.out == "Run created: http://mock-api/mock/p123/runs/r123\n" + expected = "http://mock-api/mock/p123/runs/r123" + assert actual == expected @responses.activate def test_submit_exception_handling( - self, monkeypatch, test_connection, capsys, valid_json_file + self, monkeypatch, test_connection, valid_json_file ): monkeypatch.setattr(commands, "get_project_id", lambda api, project: "p123") @@ -83,7 +81,7 @@ def test_submit_exception_handling( status=404, ) - commands.submit(test_connection, valid_json_file, "project name") + with pytest.raises(RuntimeError) as error: + commands.submit(test_connection, valid_json_file, "project name") - captured = capsys.readouterr() - assert "Error: Couldn't create run (404)" in captured.err + assert "Error: Couldn't create run (404)" in f"{error.value}" diff --git a/transcriptic/cli.py b/transcriptic/cli.py index 469633a2..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") diff --git a/transcriptic/commands.py b/transcriptic/commands.py index 03c24273..524bbcc4 100644 --- a/transcriptic/commands.py +++ b/transcriptic/commands.py @@ -25,7 +25,7 @@ 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 @@ -40,6 +40,7 @@ def submit( ): """ Submit your run to the project specified. + If successful, returns the formatted url link to the created run. Parameters ---------- @@ -57,25 +58,22 @@ def submit( 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 valid_project_id = get_project_id(api, project) if not valid_project_id: - print_stderr(f"Invalid project {project} specified") - return + raise RuntimeError(f"Invalid project {project} specified") with click.open_file(file, "r") as f: try: protocol = json.loads(f.read()) except ValueError: - print_stderr( + raise RuntimeError( "Error: Could not submit since your manifest.json " "file is improperly formatted." ) - return try: req_json = api.submit_run( @@ -87,9 +85,9 @@ def submit( ) run_id = req_json["id"] formatted_url = api.url(f"{valid_project_id}/runs/{run_id}") - click.echo(f"Run created: {formatted_url}") - except Exception as err: - print_stderr(str(err)) + return formatted_url + except AnalysisException as e: + raise RuntimeError(e.message) def release(api, name=None, package=None): From f82dbacf9d5954d904478a44469d4988913595d1 Mon Sep 17 00:00:00 2001 From: yangchoo Date: Tue, 2 Feb 2021 01:14:12 -0800 Subject: [PATCH 5/6] pin numpy version for incompat reason --- setup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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", From a5016364328a508b67e57699f026309924c32e36 Mon Sep 17 00:00:00 2001 From: yangchoo Date: Tue, 2 Feb 2021 01:18:17 -0800 Subject: [PATCH 6/6] update changelog --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 89e57605..a85179a6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -25,6 +25,7 @@ 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 ~~~~~~~