From 50beb334dcd561f9bb7c446ebde80136ef21426f Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 27 Mar 2025 11:07:25 -0700 Subject: [PATCH] Add tests for adding array element arguments and fix the code under test (#303). --- borgmatic/commands/arguments.py | 58 ++++-- tests/integration/commands/test_arguments.py | 36 ++++ tests/unit/actions/test_repo_create.py | 54 ++++- tests/unit/commands/test_arguments.py | 206 +++++++++++++++++++ 4 files changed, 331 insertions(+), 23 deletions(-) diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index 97dc3361..8a92efb9 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -299,31 +299,32 @@ def make_argument_description(schema, flag_name): ''' description = schema.get('description') schema_type = schema.get('type') + example = schema.get('example') if not description: return None - if schema_type == 'array': + if '[0]' in flag_name: + description += ' To specify a different list element, replace the "[0]" with another array index ("[1]", "[2]", etc.).' + + if example and schema_type == 'array': example_buffer = io.StringIO() yaml = ruamel.yaml.YAML(typ='safe') yaml.default_flow_style = True - yaml.dump(schema.get('example'), example_buffer) + yaml.dump(example, example_buffer) description += f' Example value: "{example_buffer.getvalue().strip()}"' - if '[0]' in flag_name: - description += ' To specify a different list element, replace the "[0]" with another array index ("[1]", "[2]", etc.).' - description = description.replace('%', '%%') return description -def add_array_element_arguments_from_schema(arguments_group, schema, unparsed_arguments, flag_name): +def add_array_element_arguments(arguments_group, unparsed_arguments, flag_name): r''' - Given an argparse._ArgumentGroup instance, a configuration schema dict, a sequence of unparsed - argument strings, and a dotted flag name, convert the schema into corresponding command-line - array element flags that correspond to the given unparsed arguments. + Given an argparse._ArgumentGroup instance, a sequence of unparsed argument strings, and a dotted + flag name, convert the schema into corresponding command-line array element flags that + correspond to the given unparsed arguments. Here's the background. We want to support flags that can have arbitrary indices like: @@ -356,26 +357,39 @@ def add_array_element_arguments_from_schema(arguments_group, schema, unparsed_ar argument will match it when parsing is performed! In this manner, we're using the actual user CLI input to inform what exact flags we support! ''' - if '[0]' not in flag_name or '--help' in unparsed_arguments: + if '[0]' not in flag_name or not unparsed_arguments or '--help' in unparsed_arguments: return pattern = re.compile(fr'^--{flag_name.replace("[0]", r"\[\d+\]").replace(".", r"\.")}$') - existing_flags = set( - itertools.chain( - *(group_action.option_strings for group_action in arguments_group._group_actions) + + # Find an existing list index flag (and its action) corresponding to the given flag name. If one + # isn't found, bail. + try: + (argument_action, existing_flag_name) = next( + (action, action_flag_name) + for action in arguments_group._group_actions + for action_flag_name in action.option_strings + if pattern.match(action_flag_name) + if f'--{flag_name}'.startswith(action_flag_name) ) - ) + except StopIteration: + return for unparsed in unparsed_arguments: unparsed_flag_name = unparsed.split('=', 1)[0] - if pattern.match(unparsed_flag_name) and unparsed_flag_name not in existing_flags: - arguments_group.add_argument( - unparsed_flag_name, - type=argument_type, - metavar=metavar, - help=description, - ) + if not pattern.match(unparsed_flag_name) or unparsed_flag_name == existing_flag_name: + continue + + arguments_group.add_argument( + unparsed_flag_name, + choices=argument_action.choices, + default=argument_action.default, + dest=unparsed_flag_name.lstrip('-'), + nargs=argument_action.nargs, + required=argument_action.nargs, + type=argument_action.type, + ) def add_arguments_from_schema(arguments_group, schema, unparsed_arguments, names=None): @@ -482,7 +496,7 @@ def add_arguments_from_schema(arguments_group, schema, unparsed_arguments, names help=description, ) - add_array_element_arguments_from_schema(arguments_group, schema, unparsed_arguments, flag_name) + add_array_element_arguments(arguments_group, unparsed_arguments, flag_name) def make_parsers(schema, unparsed_arguments): diff --git a/tests/integration/commands/test_arguments.py b/tests/integration/commands/test_arguments.py index 3469858c..5f539660 100644 --- a/tests/integration/commands/test_arguments.py +++ b/tests/integration/commands/test_arguments.py @@ -4,6 +4,42 @@ from flexmock import flexmock from borgmatic.commands import arguments as module +def test_make_argument_description_with_array_adds_example(): + assert module.make_argument_description( + schema={ + 'description': 'Thing.', + 'type': 'array', + 'example': [1, '- foo', {'bar': 'baz'}], + }, + flag_name='flag', + ) == 'Thing. Example value: "[1, \'- foo\', bar: baz]"' + + +def test_add_array_element_arguments_adds_arguments_for_array_index_flags(): + parser = module.ArgumentParser(allow_abbrev=False, add_help=False) + arguments_group = parser.add_argument_group('arguments') + arguments_group.add_argument( + '--foo[0].val', + dest='--foo[0].val', + ) + + flexmock(arguments_group).should_receive('add_argument').with_args( + '--foo[25].val', + choices=object, + default=object, + dest='foo[25].val', + nargs=object, + required=object, + type=object, + ).once() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo[25].val', 'fooval', '--bar[1].val', 'barval'), + flag_name='foo[0].val', + ) + + def test_parse_arguments_with_no_arguments_uses_defaults(): config_paths = ['default'] flexmock(module.collect).should_receive('get_default_config_paths').and_return(config_paths) diff --git a/tests/unit/actions/test_repo_create.py b/tests/unit/actions/test_repo_create.py index 8bb350b1..035dd623 100644 --- a/tests/unit/actions/test_repo_create.py +++ b/tests/unit/actions/test_repo_create.py @@ -1,9 +1,10 @@ +import pytest from flexmock import flexmock from borgmatic.actions import repo_create as module -def test_run_repo_create_does_not_raise(): +def test_run_repo_create_with_encryption_mode_argument_does_not_raise(): flexmock(module.logger).answer = lambda message: None flexmock(module.borgmatic.config.validate).should_receive('repositories_match').and_return(True) flexmock(module.borgmatic.borg.repo_create).should_receive('create_repository') @@ -28,6 +29,57 @@ def test_run_repo_create_does_not_raise(): ) +def test_run_repo_create_with_encryption_mode_option_does_not_raise(): + flexmock(module.logger).answer = lambda message: None + flexmock(module.borgmatic.config.validate).should_receive('repositories_match').and_return(True) + flexmock(module.borgmatic.borg.repo_create).should_receive('create_repository') + arguments = flexmock( + encryption_mode=None, + source_repository=flexmock(), + repository=flexmock(), + copy_crypt_key=flexmock(), + append_only=flexmock(), + storage_quota=flexmock(), + make_parent_dirs=flexmock(), + ) + + module.run_repo_create( + repository={'path': 'repo', 'encryption': flexmock()}, + config={}, + local_borg_version=None, + repo_create_arguments=arguments, + global_arguments=flexmock(dry_run=False), + local_path=None, + remote_path=None, + ) + + +def test_run_repo_create_without_encryption_mode_raises(): + flexmock(module.logger).answer = lambda message: None + flexmock(module.borgmatic.config.validate).should_receive('repositories_match').and_return(True) + flexmock(module.borgmatic.borg.repo_create).should_receive('create_repository') + arguments = flexmock( + encryption_mode=None, + source_repository=flexmock(), + repository=flexmock(), + copy_crypt_key=flexmock(), + append_only=flexmock(), + storage_quota=flexmock(), + make_parent_dirs=flexmock(), + ) + + with pytest.raises(ValueError): + module.run_repo_create( + repository={'path': 'repo'}, + config={}, + local_borg_version=None, + repo_create_arguments=arguments, + global_arguments=flexmock(dry_run=False), + local_path=None, + remote_path=None, + ) + + def test_run_repo_create_bails_if_repository_does_not_match(): flexmock(module.logger).answer = lambda message: None flexmock(module.borgmatic.config.validate).should_receive('repositories_match').and_return( diff --git a/tests/unit/commands/test_arguments.py b/tests/unit/commands/test_arguments.py index d10cf887..379dba3b 100644 --- a/tests/unit/commands/test_arguments.py +++ b/tests/unit/commands/test_arguments.py @@ -575,3 +575,209 @@ def test_parse_arguments_for_actions_raises_error_when_no_action_is_specified(): with pytest.raises(ValueError): module.parse_arguments_for_actions(('config',), action_parsers, global_parser) + + +def test_make_argument_description_without_description_bails(): + assert module.make_argument_description( + schema={ + 'description': None, + 'type': 'not yours', + }, + flag_name='flag', + ) is None + + +def test_make_argument_description_with_array_adds_example(): + buffer = flexmock() + buffer.should_receive('getvalue').and_return('[example]') + flexmock(module.io).should_receive('StringIO').and_return(buffer) + yaml = flexmock() + yaml.should_receive('dump') + flexmock(module.ruamel.yaml).should_receive('YAML').and_return(yaml) + + assert module.make_argument_description( + schema={ + 'description': 'Thing.', + 'type': 'array', + 'example': ['example'], + }, + flag_name='flag', + ) == 'Thing. Example value: "[example]"' + + +def test_make_argument_description_with_array_skips_missing_example(): + yaml = flexmock() + yaml.should_receive('dump').and_return('[example]') + flexmock(module.ruamel.yaml).should_receive('YAML').and_return(yaml) + + assert module.make_argument_description( + schema={ + 'description': 'Thing.', + 'type': 'array', + }, + flag_name='flag', + ) == 'Thing.' + + +def test_make_argument_description_with_array_index_in_flag_name_adds_to_description(): + assert 'list element' in module.make_argument_description( + schema={ + 'description': 'Thing.', + 'type': 'something', + }, + flag_name='flag[0]', + ) + + +def test_make_argument_description_escapes_percent_character(): + assert module.make_argument_description( + schema={ + 'description': '% Thing.', + 'type': 'something', + }, + flag_name='flag', + ) == '%% Thing.' + + +def test_add_array_element_arguments_without_array_index_bails(): + arguments_group = flexmock() + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=(), + flag_name='foo', + ) + + +def test_add_array_element_arguments_with_help_flag_bails(): + arguments_group = flexmock() + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo', '--help', '--bar'), + flag_name='foo[0]', + ) + + +def test_add_array_element_arguments_without_any_flags_bails(): + arguments_group = flexmock() + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=(), + flag_name='foo[0]', + ) + + +def test_add_array_element_arguments_without_array_index_flags_bails(): + arguments_group = flexmock( + _group_actions=( + flexmock( + option_strings=('--foo[0].val',), + ), + ) + ) + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo', '--bar'), + flag_name='foo[0].val', + ) + + +def test_add_array_element_arguments_with_non_matching_array_index_flags_bails(): + arguments_group = flexmock( + _group_actions=( + flexmock( + option_strings=('--foo[0].val',), + ), + ) + ) + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo', '--bar[25].val', 'barval'), + flag_name='foo[0].val', + ) + + +def test_add_array_element_arguments_with_identical_array_index_flag_bails(): + arguments_group = flexmock( + _group_actions=( + flexmock( + option_strings=('--foo[0].val',), + ), + ) + ) + arguments_group.should_receive('add_argument').never() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo[0].val', 'fooval', '--bar'), + flag_name='foo[0].val', + ) + + +def test_add_array_element_arguments_adds_arguments_for_array_index_flags(): + arguments_group = flexmock( + _group_actions=( + flexmock( + option_strings=('--foo[0].val',), + choices=flexmock(), + default=flexmock(), + nargs=flexmock(), + required=flexmock(), + type=flexmock(), + ), + ) + ) + arguments_group.should_receive('add_argument').with_args( + '--foo[25].val', + choices=object, + default=object, + dest='foo[25].val', + nargs=object, + required=object, + type=object, + ).once() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo[25].val', 'fooval', '--bar[1].val', 'barval'), + flag_name='foo[0].val', + ) + + +def test_add_array_element_arguments_adds_arguments_for_array_index_flags_with_equals_sign(): + arguments_group = flexmock( + _group_actions=( + flexmock( + option_strings=('--foo[0].val',), + choices=flexmock(), + default=flexmock(), + nargs=flexmock(), + required=flexmock(), + type=flexmock(), + ), + ) + ) + arguments_group.should_receive('add_argument').with_args( + '--foo[25].val', + choices=object, + default=object, + dest='foo[25].val', + nargs=object, + required=object, + type=object, + ).once() + + module.add_array_element_arguments( + arguments_group=arguments_group, + unparsed_arguments=('--foo[25].val=fooval', '--bar[1].val=barval'), + flag_name='foo[0].val', + )