From 5bf2f546b9db76ed2863fcb9e982adb15f5983f0 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 27 Mar 2025 21:01:56 -0700 Subject: [PATCH] More automated tests (#303). --- borgmatic/commands/arguments.py | 5 +- borgmatic/config/arguments.py | 51 ++--- borgmatic/config/schema.py | 25 ++- tests/integration/commands/test_arguments.py | 8 +- tests/integration/config/test_arguments.py | 35 ++++ tests/unit/commands/test_arguments.py | 50 +++-- tests/unit/config/test_arguments.py | 195 +++++++++++++++++++ 7 files changed, 310 insertions(+), 59 deletions(-) create mode 100644 tests/integration/config/test_arguments.py create mode 100644 tests/unit/config/test_arguments.py diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index 3bd22082..140dc1ae 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -484,7 +484,10 @@ def add_arguments_from_schema(arguments_group, schema, unparsed_arguments, names metavar = names[-1].upper() description = make_argument_description(schema, flag_name) - argument_type = borgmatic.config.schema.parse_type(schema_type) + + # array=str instead of list here to support specifying a list as a YAML string on the + # command-line. + argument_type = borgmatic.config.schema.parse_type(schema_type, array=str) full_flag_name = f"--{flag_name.replace('_', '-')}" # As a UX nicety, allow boolean options that have a default of false to have command-line flags diff --git a/borgmatic/config/arguments.py b/borgmatic/config/arguments.py index 8f7023cd..85c8eef9 100644 --- a/borgmatic/config/arguments.py +++ b/borgmatic/config/arguments.py @@ -3,6 +3,8 @@ import re import ruamel.yaml +import borgmatic.config.schema + LIST_INDEX_KEY_PATTERN = re.compile(r'^(?P[a-zA-z-]+)\[(?P\d+)\]$') @@ -45,7 +47,7 @@ def set_values(config, keys, value): try: set_values(config[list_key][list_index], keys[1:], value) except IndexError: - raise ValueError(f'The list index {first_key} is out of range') + raise ValueError(f'Argument list index {first_key} is out of range') return @@ -99,6 +101,7 @@ def convert_value_type(value, option_type): And if the source value isn't a string, return it as-is. Raise ruamel.yaml.error.YAMLError if there's a parse issue with the YAML. + Raise ValueError if the parsed value doesn't match the option type. ''' if not isinstance(value, str): return value @@ -106,7 +109,15 @@ def convert_value_type(value, option_type): if option_type == 'string': return value - return ruamel.yaml.YAML(typ='safe').load(io.StringIO(value)) + try: + parsed_value = ruamel.yaml.YAML(typ='safe').load(io.StringIO(value)) + except ruamel.yaml.error.YAMLError as error: + raise ValueError(f'Argument value "{value}" is invalid: {error.problem}') + + if not isinstance(parsed_value, borgmatic.config.schema.parse_type(option_type)): + raise ValueError(f'Argument value "{value}" is not of the expected type: {option_type}') + + return parsed_value def prepare_arguments_for_config(global_arguments, schema): @@ -122,39 +133,34 @@ def prepare_arguments_for_config(global_arguments, schema): ( (('my_option', 'sub_option'), 'value1'), - (('other_option'), 'value2'), + (('other_option',), 'value2'), ) - - Raise ValueError if an override can't be parsed. ''' prepared_values = [] for argument_name, value in global_arguments.__dict__.items(): - try: - if value is None: - continue + if value is None: + continue - keys = tuple(argument_name.split('.')) - option_type = type_for_option(schema, keys) + keys = tuple(argument_name.split('.')) + option_type = type_for_option(schema, keys) - # The argument doesn't correspond to any option in the schema, so ignore it. It's - # probably a flag that borgmatic has on the command-line but not in configuration. - if option_type is None: - continue + # The argument doesn't correspond to any option in the schema, so ignore it. It's + # probably a flag that borgmatic has on the command-line but not in configuration. + if option_type is None: + continue - prepared_values.append( - ( - keys, - convert_value_type(value, option_type), - ) + prepared_values.append( + ( + keys, + convert_value_type(value, option_type), ) - except ruamel.yaml.error.YAMLError as error: - raise ValueError(f'Invalid override "{argument_name}": {error.problem}') + ) return tuple(prepared_values) -def apply_arguments_to_config(config, schema, global_arguments): +def apply_arguments_to_config(config, schema, global_arguments): # pragma: no cover ''' Given a configuration dict, a corresponding configuration schema dict, and global arguments as an argparse.Namespace, set those given argument values into their corresponding configuration @@ -164,6 +170,5 @@ def apply_arguments_to_config(config, schema, global_arguments): configuration object. Additionally, flags like "--foo.bar[0].baz" are supported to update a list element in the configuration. ''' - for keys, value in prepare_arguments_for_config(global_arguments, schema): set_values(config, keys, value) diff --git a/borgmatic/config/schema.py b/borgmatic/config/schema.py index 52358a3e..6981dde5 100644 --- a/borgmatic/config/schema.py +++ b/borgmatic/config/schema.py @@ -23,22 +23,27 @@ def get_properties(schema): return schema.get('properties', {}) -def parse_type(schema_type): +def parse_type(schema_type, **overrides): ''' Given a schema type as a string, return the corresponding Python type. + If any overrides are given in the from of a schema type string to a Python type, then override + the default type mapping with them. + Raise ValueError if the schema type is unknown. ''' try: - return { - 'string': str, - 'integer': int, - 'number': decimal.Decimal, - 'boolean': bool, - # This is str instead of list to support specifying a list as a YAML string on the - # command-line. - 'array': str, - }[schema_type] + return dict( + { + 'array': list, + 'boolean': bool, + 'integer': int, + 'number': decimal.Decimal, + 'object': dict, + 'string': str, + }, + **overrides, + )[schema_type] except KeyError: raise ValueError(f'Unknown type in configuration schema: {schema_type}') diff --git a/tests/integration/commands/test_arguments.py b/tests/integration/commands/test_arguments.py index 200368aa..032b9495 100644 --- a/tests/integration/commands/test_arguments.py +++ b/tests/integration/commands/test_arguments.py @@ -71,9 +71,9 @@ def test_add_arguments_from_schema_with_nested_object_adds_flag_for_each_option( 'properties': { 'bar': {'type': 'integer', 'description': 'help 1'}, 'baz': {'type': 'string', 'description': 'help 2'}, - } + }, } - } + }, }, unparsed_arguments=(), ) @@ -109,11 +109,11 @@ def test_add_arguments_from_schema_with_array_and_nested_object_adds_multiple_fl 'type': 'integer', 'description': 'help 1', } - } + }, }, 'description': 'help 2', } - } + }, }, unparsed_arguments=(), ) diff --git a/tests/integration/config/test_arguments.py b/tests/integration/config/test_arguments.py new file mode 100644 index 00000000..449964ee --- /dev/null +++ b/tests/integration/config/test_arguments.py @@ -0,0 +1,35 @@ +import pytest +from flexmock import flexmock + +from borgmatic.config import arguments as module + + +def test_convert_value_type_passes_through_non_string_value(): + assert module.convert_value_type([1, 2], 'array') == [1, 2] + + +def test_convert_value_type_passes_through_string_option_type(): + assert module.convert_value_type('foo', 'string') == 'foo' + + +def test_convert_value_type_parses_array_option_type(): + assert module.convert_value_type('[foo, bar]', 'array') == ['foo', 'bar'] + + +def test_convert_value_type_with_array_option_type_and_no_array_raises(): + with pytest.raises(ValueError): + module.convert_value_type('{foo, bar}', 'array') + + +def test_convert_value_type_parses_object_option_type(): + assert module.convert_value_type('{foo: bar}', 'object') == {'foo': 'bar'} + + +def test_convert_value_type_with_invalid_value_raises(): + with pytest.raises(ValueError): + module.convert_value_type('{foo, bar', 'object') + + +def test_convert_value_type_with_unknown_option_type_raises(): + with pytest.raises(ValueError): + module.convert_value_type('{foo, bar}', 'thingy') diff --git a/tests/unit/commands/test_arguments.py b/tests/unit/commands/test_arguments.py index fcfa9589..1e5bbe0b 100644 --- a/tests/unit/commands/test_arguments.py +++ b/tests/unit/commands/test_arguments.py @@ -859,8 +859,12 @@ def test_add_arguments_from_schema_with_non_dict_schema_bails(): def test_add_arguments_from_schema_with_nested_object_adds_flag_for_each_option(): arguments_group = flexmock() - flexmock(module).should_receive('make_argument_description').and_return('help 1').and_return('help 2') - flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(int).and_return(str) + flexmock(module).should_receive('make_argument_description').and_return('help 1').and_return( + 'help 2' + ) + flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return( + int + ).and_return(str) arguments_group.should_receive('add_argument').with_args( '--foo.bar', type=int, @@ -885,9 +889,9 @@ def test_add_arguments_from_schema_with_nested_object_adds_flag_for_each_option( 'properties': { 'bar': {'type': 'integer'}, 'baz': {'type': 'str'}, - } + }, } - } + }, }, unparsed_arguments=(), ) @@ -914,9 +918,9 @@ def test_add_arguments_from_schema_uses_first_non_null_type_from_multi_type_obje 'type': ['null', 'object', 'boolean'], 'properties': { 'bar': {'type': 'integer'}, - } + }, } - } + }, }, unparsed_arguments=(), ) @@ -939,9 +943,9 @@ def test_add_arguments_from_schema_with_empty_multi_type_raises(): 'type': [], 'properties': { 'bar': {'type': 'integer'}, - } + }, } - } + }, }, unparsed_arguments=(), ) @@ -962,7 +966,7 @@ def test_add_arguments_from_schema_with_propertyless_option_does_not_add_flag(): 'foo': { 'type': 'object', } - } + }, }, unparsed_arguments=(), ) @@ -989,9 +993,9 @@ def test_add_arguments_from_schema_with_array_adds_flag(): 'type': 'array', 'items': { 'type': 'integer', - } + }, } - } + }, }, unparsed_arguments=(), ) @@ -999,8 +1003,12 @@ def test_add_arguments_from_schema_with_array_adds_flag(): def test_add_arguments_from_schema_with_array_and_nested_object_adds_multiple_flags(): arguments_group = flexmock() - flexmock(module).should_receive('make_argument_description').and_return('help 1').and_return('help 2') - flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(int).and_return(str) + flexmock(module).should_receive('make_argument_description').and_return('help 1').and_return( + 'help 2' + ) + flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return( + int + ).and_return(str) arguments_group.should_receive('add_argument').with_args( '--foo[0].bar', type=int, @@ -1033,10 +1041,10 @@ def test_add_arguments_from_schema_with_array_and_nested_object_adds_multiple_fl 'bar': { 'type': 'integer', } - } - } + }, + }, } - } + }, }, unparsed_arguments=(), ) @@ -1063,7 +1071,7 @@ def test_add_arguments_from_schema_with_default_false_boolean_adds_valueless_fla 'type': 'boolean', 'default': False, } - } + }, }, unparsed_arguments=(), ) @@ -1090,7 +1098,7 @@ def test_add_arguments_from_schema_with_default_true_boolean_adds_value_flag(): 'type': 'boolean', 'default': True, } - } + }, }, unparsed_arguments=(), ) @@ -1116,7 +1124,7 @@ def test_add_arguments_from_schema_with_defaultless_boolean_adds_value_flag(): 'foo': { 'type': 'boolean', } - } + }, }, unparsed_arguments=(), ) @@ -1151,7 +1159,7 @@ def test_add_arguments_from_schema_skips_omitted_flag_name(): 'foo': { 'type': 'string', }, - } + }, }, unparsed_arguments=(), ) @@ -1177,7 +1185,7 @@ def test_add_arguments_from_schema_rewrites_option_name_to_flag_name(): 'foo_and_stuff': { 'type': 'string', }, - } + }, }, unparsed_arguments=(), ) diff --git a/tests/unit/config/test_arguments.py b/tests/unit/config/test_arguments.py new file mode 100644 index 00000000..25c8873f --- /dev/null +++ b/tests/unit/config/test_arguments.py @@ -0,0 +1,195 @@ +import pytest +from flexmock import flexmock + +from borgmatic.config import arguments as module + + +def test_set_values_without_keys_bails(): + config = {'option': 'value'} + module.set_values(config=config, keys=(), value=5) + + assert config == {'option': 'value'} + + +def test_set_values_with_keys_adds_them_to_config(): + config = {'option': 'value'} + + module.set_values(config=config, keys=('foo', 'bar', 'baz'), value=5) + + assert config == {'option': 'value', 'foo': {'bar': {'baz': 5}}} + + +def test_set_values_with_one_existing_key_adds_others_to_config(): + config = {'foo': {'other': 'value'}} + + module.set_values(config=config, keys=('foo', 'bar', 'baz'), value=5) + + assert config == {'foo': {'other': 'value', 'bar': {'baz': 5}}} + + +def test_set_values_with_two_existing_keys_adds_others_to_config(): + config = {'foo': {'bar': {'other': 'value'}}} + + module.set_values(config=config, keys=('foo', 'bar', 'baz'), value=5) + + assert config == {'foo': {'bar': {'other': 'value', 'baz': 5}}} + + +def test_set_values_with_list_index_key_adds_it_to_config(): + config = {'foo': {'bar': [{'option': 'value'}, {'other': 'thing'}]}} + + module.set_values(config=config, keys=('foo', 'bar[1]', 'baz'), value=5) + + assert config == {'foo': {'bar': [{'option': 'value'}, {'other': 'thing', 'baz': 5}]}} + + +def test_set_values_with_list_index_key_out_of_range_raises(): + config = {'foo': {'bar': [{'option': 'value'}]}} + + with pytest.raises(ValueError): + module.set_values(config=config, keys=('foo', 'bar[1]', 'baz'), value=5) + + +def test_set_values_with_list_index_key_missing_list_and_out_of_range_raises(): + config = {'other': 'value'} + + with pytest.raises(ValueError): + module.set_values(config=config, keys=('foo', 'bar[1]', 'baz'), value=5) + + +def test_set_values_with_final_list_index_key_adds_it_to_config(): + config = {'foo': {'bar': [1, 2]}} + + module.set_values(config=config, keys=('foo', 'bar[1]'), value=5) + + assert config == {'foo': {'bar': [1, 5]}} + + +def test_type_for_option_with_option_finds_type(): + assert ( + module.type_for_option( + schema={'type': 'object', 'properties': {'foo': {'type': 'integer'}}}, + option_keys=('foo',), + ) + == 'integer' + ) + + +def test_type_for_option_with_nested_option_finds_type(): + assert ( + module.type_for_option( + schema={ + 'type': 'object', + 'properties': { + 'foo': {'type': 'object', 'properties': {'bar': {'type': 'boolean'}}} + }, + }, + option_keys=('foo', 'bar'), + ) + == 'boolean' + ) + + +def test_type_for_option_with_missing_nested_option_finds_nothing(): + assert ( + module.type_for_option( + schema={ + 'type': 'object', + 'properties': { + 'foo': {'type': 'object', 'properties': {'other': {'type': 'integer'}}} + }, + }, + option_keys=('foo', 'bar'), + ) + is None + ) + + +def test_type_for_option_with_typeless_nested_option_finds_nothing(): + assert ( + module.type_for_option( + schema={ + 'type': 'object', + 'properties': {'foo': {'type': 'object', 'properties': {'bar': {'example': 5}}}}, + }, + option_keys=('foo', 'bar'), + ) + is None + ) + + +def test_type_for_list_index_option_finds_type(): + assert ( + module.type_for_option( + schema={ + 'type': 'object', + 'properties': {'foo': {'type': 'array', 'items': {'type': 'integer'}}}, + }, + option_keys=('foo[0]',), + ) + == 'integer' + ) + + +def test_type_for_nested_list_index_option_finds_type(): + assert ( + module.type_for_option( + schema={ + 'type': 'object', + 'properties': { + 'foo': { + 'type': 'array', + 'items': {'type': 'object', 'properties': {'bar': {'type': 'integer'}}}, + } + }, + }, + option_keys=('foo[0]', 'bar'), + ) + == 'integer' + ) + + +def test_prepare_arguments_for_config_converts_arguments_to_keys(): + assert module.prepare_arguments_for_config( + global_arguments=flexmock(**{'my_option.sub_option': 'value1', 'other_option': 'value2'}), + schema={ + 'type': 'object', + 'properties': { + 'my_option': {'type': 'object', 'properties': {'sub_option': {'type': 'string'}}}, + 'other_option': {'type': 'string'}, + }, + }, + ) == ( + (('my_option', 'sub_option'), 'value1'), + (('other_option',), 'value2'), + ) + + +def test_prepare_arguments_for_config_skips_option_with_none_value(): + assert module.prepare_arguments_for_config( + global_arguments=flexmock(**{'my_option.sub_option': None, 'other_option': 'value2'}), + schema={ + 'type': 'object', + 'properties': { + 'my_option': {'type': 'object', 'properties': {'sub_option': {'type': 'string'}}}, + 'other_option': {'type': 'string'}, + }, + }, + ) == ( + (('other_option',), 'value2'), + ) + + +def test_prepare_arguments_for_config_skips_option_missing_from_schema(): + assert module.prepare_arguments_for_config( + global_arguments=flexmock(**{'my_option.sub_option': 'value1', 'other_option': 'value2'}), + schema={ + 'type': 'object', + 'properties': { + 'my_option': {'type': 'object'}, + 'other_option': {'type': 'string'}, + }, + }, + ) == ( + (('other_option',), 'value2'), + )