From 1ce9d2eb2c28ed2cf374fa75e29a49f98d223122 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Thu, 24 Mar 2022 14:47:42 +0100 Subject: [PATCH] CLI: do not except when `--help` is passed to sub command The `rocrate add workflow --help` call would except because the `add` command group would attempt to instantiate a research object crate. But since no existing crate was defined through the `-c` option, the instantiation of the crate would except. There is no straightforward way in `click` to prevent the execution of group command code when `--help` is passed. Instead, we move the addition of the `-c/--crate-dir` option to those commands that actually need it. Code duplication is prevented by defining the option once, assigning it to `OPTION_CRATE_PATH` and using that decorator for each command that requires the option. This is a breaking change for the interface as the crate path should now no longer be specified directly after the main command but rather after the actual leaf command. So: rocrate -c some/ro-crate add workflow Now becomes rocrate add workflow -c some/ro-crate The advantage of this is that it doesn't enforce this option on all commands, even those that don't need it and it is probably more intuitive for the user to put the option closer to all other options. --- rocrate/cli.py | 76 +++++++++++++++++++------------------ test/test_cli.py | 98 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 113 insertions(+), 61 deletions(-) diff --git a/rocrate/cli.py b/rocrate/cli.py index a74ec90..16f9196 100644 --- a/rocrate/cli.py +++ b/rocrate/cli.py @@ -54,58 +54,58 @@ def convert(self, value, param, ctx): CSV = CSVParamType() +OPTION_CRATE_PATH = click.option('-c', '--crate-dir', type=click.Path(), default=os.getcwd) + @click.group() -@click.option('-c', '--crate-dir', type=click.Path()) -@click.pass_context -def cli(ctx, crate_dir): - ctx.obj = state = State() - state.crate_dir = os.getcwd() if not crate_dir else os.path.abspath(crate_dir) +def cli(): + pass @cli.command() @click.option('--gen-preview', is_flag=True) @click.option('-e', '--exclude', type=CSV) -@click.pass_obj -def init(state, gen_preview, exclude): - crate = ROCrate(state.crate_dir, init=True, gen_preview=gen_preview, exclude=exclude) - crate.metadata.write(state.crate_dir) +@OPTION_CRATE_PATH +def init(crate_dir, gen_preview, exclude): + crate = ROCrate(crate_dir, init=True, gen_preview=gen_preview, exclude=exclude) + crate.metadata.write(crate_dir) if crate.preview: - crate.preview.write(state.crate_dir) + crate.preview.write(crate_dir) @cli.group() -@click.pass_obj -def add(state): - state.crate = ROCrate(state.crate_dir, init=False, gen_preview=False) +def add(): + pass @add.command() @click.argument('path', type=click.Path(exists=True)) @click.option('-l', '--language', type=click.Choice(LANG_CHOICES), default="cwl") -@click.pass_obj -def workflow(state, path, language): +@OPTION_CRATE_PATH +def workflow(crate_dir, path, language): + crate = ROCrate(crate_dir, init=False, gen_preview=False) source = Path(path).resolve(strict=True) try: - dest_path = source.relative_to(state.crate_dir) + dest_path = source.relative_to(crate_dir) except ValueError: # For now, only support marking an existing file as a workflow - raise ValueError(f"{source} is not in the crate dir {state.crate_dir}") + raise ValueError(f"{source} is not in the crate dir {crate_dir}") # TODO: add command options for main and gen_cwl - state.crate.add_workflow(source, dest_path, main=True, lang=language, gen_cwl=False) - state.crate.metadata.write(state.crate_dir) + crate.add_workflow(source, dest_path, main=True, lang=language, gen_cwl=False) + crate.metadata.write(crate_dir) @add.command(name="test-suite") @click.option('-i', '--identifier') @click.option('-n', '--name') @click.option('-m', '--main-entity') -@click.pass_obj -def suite(state, identifier, name, main_entity): - suite_ = state.crate.add_test_suite(identifier=add_hash(identifier), name=name, main_entity=main_entity) - state.crate.metadata.write(state.crate_dir) - print(suite_.id) +@OPTION_CRATE_PATH +def suite(crate_dir, identifier, name, main_entity): + crate = ROCrate(crate_dir, init=False, gen_preview=False) + suite = crate.add_test_suite(identifier=add_hash(identifier), name=name, main_entity=main_entity) + crate.metadata.write(crate_dir) + print(suite.id) @add.command(name="test-instance") @@ -115,13 +115,14 @@ def suite(state, identifier, name, main_entity): @click.option('-s', '--service', type=click.Choice(SERVICE_CHOICES), default="jenkins") @click.option('-i', '--identifier') @click.option('-n', '--name') -@click.pass_obj -def instance(state, suite, url, resource, service, identifier, name): - instance_ = state.crate.add_test_instance( +@OPTION_CRATE_PATH +def instance(crate_dir, suite, url, resource, service, identifier, name): + crate = ROCrate(crate_dir, init=False, gen_preview=False) + instance_ = crate.add_test_instance( add_hash(suite), url, resource=resource, service=service, identifier=add_hash(identifier), name=name ) - state.crate.metadata.write(state.crate_dir) + crate.metadata.write(crate_dir) print(instance_.id) @@ -130,23 +131,24 @@ def instance(state, suite, url, resource, service, identifier, name): @click.argument('path', type=click.Path(exists=True)) @click.option('-e', '--engine', type=click.Choice(ENGINE_CHOICES), default="planemo") @click.option('-v', '--engine-version') -@click.pass_obj -def definition(state, suite, path, engine, engine_version): +@OPTION_CRATE_PATH +def definition(crate_dir, suite, path, engine, engine_version): + crate = ROCrate(crate_dir, init=False, gen_preview=False) source = Path(path).resolve(strict=True) try: - dest_path = source.relative_to(state.crate_dir) + dest_path = source.relative_to(crate_dir) except ValueError: # For now, only support marking an existing file as a test definition - raise ValueError(f"{source} is not in the crate dir {state.crate_dir}") - state.crate.add_test_definition(suite, source=source, dest_path=dest_path, engine=engine, engine_version=engine_version) - state.crate.metadata.write(state.crate_dir) + raise ValueError(f"{source} is not in the crate dir {crate_dir}") + crate.add_test_definition(suite, source=source, dest_path=dest_path, engine=engine, engine_version=engine_version) + crate.metadata.write(crate_dir) @cli.command() @click.argument('dst', type=click.Path(writable=True)) -@click.pass_obj -def write_zip(state, dst): - crate = ROCrate(state.crate_dir, init=False, gen_preview=False) +@OPTION_CRATE_PATH +def write_zip(crate_dir, dst): + crate = ROCrate(crate_dir, init=False, gen_preview=False) crate.write_zip(dst) diff --git a/test/test_cli.py b/test/test_cli.py index adcda3b..db3f624 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -14,10 +14,11 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - import json -import pytest + +import click from click.testing import CliRunner +import pytest from rocrate.cli import cli from rocrate.model.file import File @@ -25,6 +26,55 @@ from rocrate.rocrate import ROCrate +def get_command_paths(command): + """Return a list of full command paths for all leaf commands that are part of the given root command. + + For example, for a root command that has two subcommands ``command_a`` and ``command_b`` where ``command_b`` in turn + has two subcommands, the returned list will have the form:: + + [ + ['command_a'], + ['command_b', 'subcommand_a'], + ['command_b', 'subcommand_b'], + ] + + :param command: The root command. + :return: A list of lists, where each element is the full command path to a leaf command. + """ + + def resolve_commands(command, command_path, commands): + if isinstance(command, click.MultiCommand): + for subcommand in command.commands.values(): + command_subpath = command_path + [subcommand.name] + resolve_commands(subcommand, command_subpath, commands) + else: + commands.append(command_path) + + commands = [] + resolve_commands(command, [], commands) + + return commands + + +@pytest.mark.parametrize('command_path', get_command_paths(cli)) +def test_cli_help(command_path): + """Test that invoking any CLI command with ``--help`` prints the help string and exits normally. + + This is a regression test for: https://github.com/ResearchObject/ro-crate-py/issues/97 + + Note that we cannot simply invoke the actual leaf :class:`click.Command` that we are testing, because the test + runner follows a different path then when actually invoking the command from the command line. This means that any + code that is in the groups that the command is part of, won't be executed. This in turn means that a command could + actually be broken when invoked from the command line but would not be detected by the test. The workaround is to + invoke the full command path. For example when testing ``add workflow --help``, we cannot simply invoke ``workflow`` + with ``['--help']`` as argument but we need to invoke the base command with ``['add', 'workflow', '--help']``. + """ + runner = CliRunner() + result = runner.invoke(cli, command_path + ['--help']) + assert result.exit_code == 0, result.output + assert 'Usage:' in result.output + + @pytest.mark.parametrize("gen_preview,cwd", [(False, False), (False, True), (True, False), (True, True)]) def test_cli_init(test_data_dir, helpers, monkeypatch, cwd, gen_preview): crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase" @@ -33,12 +83,11 @@ def test_cli_init(test_data_dir, helpers, monkeypatch, cwd, gen_preview): metadata_path.unlink() runner = CliRunner() - args = [] + args = ["init"] if cwd: monkeypatch.chdir(str(crate_dir)) else: args.extend(["-c", str(crate_dir)]) - args.append("init") if gen_preview: args.append("--gen-preview") result = runner.invoke(cli, args) @@ -59,8 +108,11 @@ def test_cli_init_exclude(test_data_dir, helpers): (crate_dir / helpers.METADATA_FILE_NAME).unlink() exclude = "test,README.md" runner = CliRunner() - args = ["-c", str(crate_dir), "init", "-e", exclude] - assert runner.invoke(cli, args).exit_code == 0 + args = ["init", "-c", str(crate_dir), "-e", exclude] + result = runner.invoke(cli, args) + print(result.output) + print(args) + assert result.exit_code == 0 crate = ROCrate(crate_dir) for p in "LICENSE", "sort-and-change-case.ga": assert isinstance(crate.dereference(p), File) @@ -75,20 +127,20 @@ def test_cli_add_workflow(test_data_dir, helpers, monkeypatch, cwd): # init crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase" runner = CliRunner() - assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0 + assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0 json_entities = helpers.read_json_entities(crate_dir) assert "sort-and-change-case.ga" in json_entities assert json_entities["sort-and-change-case.ga"]["@type"] == "File" # add wf_path = crate_dir / "sort-and-change-case.ga" - args = [] + args = ["add", "workflow"] if cwd: monkeypatch.chdir(str(crate_dir)) wf_path = wf_path.relative_to(crate_dir) else: args.extend(["-c", str(crate_dir)]) for lang in "cwl", "galaxy": - extra_args = ["add", "workflow", "-l", lang, str(wf_path)] + extra_args = ["-l", lang, str(wf_path)] result = runner.invoke(cli, args + extra_args) assert result.exit_code == 0 json_entities = helpers.read_json_entities(crate_dir) @@ -104,35 +156,35 @@ def test_cli_add_test_metadata(test_data_dir, helpers, monkeypatch, cwd): # init crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase" runner = CliRunner() - assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0 + assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0 json_entities = helpers.read_json_entities(crate_dir) def_id = "test/test1/sort-and-change-case-test.yml" assert def_id in json_entities assert json_entities[def_id]["@type"] == "File" # add workflow wf_path = crate_dir / "sort-and-change-case.ga" - assert runner.invoke(cli, ["-c", str(crate_dir), "add", "workflow", "-l", "galaxy", str(wf_path)]).exit_code == 0 + assert runner.invoke(cli, ["add", "workflow", "-c", str(crate_dir), "-l", "galaxy", str(wf_path)]).exit_code == 0 # add test suite - result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-suite"]) + result = runner.invoke(cli, ["add", "test-suite", "-c", str(crate_dir)]) assert result.exit_code == 0 suite_id = result.output.strip() json_entities = helpers.read_json_entities(crate_dir) assert suite_id in json_entities # add test instance - result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-instance", suite_id, "http://example.com", "-r", "jobs"]) + result = runner.invoke(cli, ["add", "test-instance", "-c", str(crate_dir), suite_id, "http://example.com", "-r", "jobs"]) assert result.exit_code == 0 instance_id = result.output.strip() json_entities = helpers.read_json_entities(crate_dir) assert instance_id in json_entities # add test definition def_path = crate_dir / def_id - args = [] + args = ["add", "test-definition"] if cwd: monkeypatch.chdir(str(crate_dir)) def_path = def_path.relative_to(crate_dir) else: args.extend(["-c", str(crate_dir)]) - extra_args = ["add", "test-definition", "-e", "planemo", "-v", ">=0.70", suite_id, str(def_path)] + extra_args = ["-e", "planemo", "-v", ">=0.70", suite_id, str(def_path)] result = runner.invoke(cli, args + extra_args) assert result.exit_code == 0 json_entities = helpers.read_json_entities(crate_dir) @@ -154,12 +206,12 @@ def test_cli_add_test_metadata(test_data_dir, helpers, monkeypatch, cwd): def test_cli_add_test_metadata_explicit_ids(test_data_dir, helpers, monkeypatch, hash_): crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase" runner = CliRunner() - assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0 + assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0 wf_path = crate_dir / "sort-and-change-case.ga" - assert runner.invoke(cli, ["-c", str(crate_dir), "add", "workflow", "-l", "galaxy", str(wf_path)]).exit_code == 0 + assert runner.invoke(cli, ["add", "workflow", "-c", str(crate_dir), "-l", "galaxy", str(wf_path)]).exit_code == 0 suite_id = "#foo" cli_suite_id = suite_id if hash_ else suite_id[1:] - result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-suite", "-i", cli_suite_id]) + result = runner.invoke(cli, ["add", "test-suite", "-c", str(crate_dir), "-i", cli_suite_id]) assert result.exit_code == 0 assert result.output.strip() == suite_id json_entities = helpers.read_json_entities(crate_dir) @@ -167,8 +219,7 @@ def test_cli_add_test_metadata_explicit_ids(test_data_dir, helpers, monkeypatch, instance_id = "#bar" cli_instance_id = instance_id if hash_ else instance_id[1:] result = runner.invoke( - cli, ["-c", str(crate_dir), "add", "test-instance", cli_suite_id, - "http://example.com", "-r", "jobs", "-i", cli_instance_id] + cli, ["add", "test-instance", cli_suite_id, "http://example.com", "-c", str(crate_dir), "-r", "jobs", "-i", cli_instance_id] ) assert result.exit_code == 0 assert result.output.strip() == instance_id @@ -180,18 +231,17 @@ def test_cli_add_test_metadata_explicit_ids(test_data_dir, helpers, monkeypatch, def test_cli_write_zip(test_data_dir, monkeypatch, cwd): crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase" runner = CliRunner() - assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0 + assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0 wf_path = crate_dir / "sort-and-change-case.ga" - args = ["-c", str(crate_dir), "add", "workflow", str(wf_path)] + args = ["add", "workflow", str(wf_path), "-c", str(crate_dir)] assert runner.invoke(cli, args).exit_code == 0 output_zip_path = test_data_dir / "test-zip-archive.zip" - args = [] + args = ["write-zip"] if cwd: monkeypatch.chdir(str(crate_dir)) else: args.extend(["-c", str(crate_dir)]) - args.append("write-zip") args.append(str(output_zip_path)) result = runner.invoke(cli, args)