From 45c114973cc41688c7a19c6ce39117fd72f2bc98 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sat, 8 Mar 2025 18:31:16 -0800 Subject: [PATCH] Add missing test coverage for new/changed code (#1019). --- borgmatic/config/normalize.py | 2 +- borgmatic/hooks/command.py | 5 +- tests/integration/config/test_generate.py | 110 ++++++++ tests/unit/config/test_generate.py | 22 +- tests/unit/config/test_normalize.py | 110 ++++++++ tests/unit/config/test_validate.py | 9 + tests/unit/hooks/test_command.py | 313 ++++++++++++++++++++++ 7 files changed, 555 insertions(+), 16 deletions(-) diff --git a/borgmatic/config/normalize.py b/borgmatic/config/normalize.py index d8244297..11f21ce0 100644 --- a/borgmatic/config/normalize.py +++ b/borgmatic/config/normalize.py @@ -58,7 +58,7 @@ def normalize_sections(config_filename, config): return [] -def make_command_hook_deprecation_log(config_filename, option_name): +def make_command_hook_deprecation_log(config_filename, option_name): # pragma: no cover ''' Given a configuration filename and the name of a configuration option, return a deprecation warning log for it. diff --git a/borgmatic/hooks/command.py b/borgmatic/hooks/command.py index ab4d8af0..4d69782f 100644 --- a/borgmatic/hooks/command.py +++ b/borgmatic/hooks/command.py @@ -88,11 +88,11 @@ def execute_hooks(command_hooks, umask, dry_run, **context): elif 'after' in hook_config: description = f'after {hook_config.get("after")}' else: - raise ValueError('Invalid hook configuration: {hook_config}') + raise ValueError(f'Invalid hook configuration: {hook_config}') if not commands: logger.debug(f'No commands to run for {description} hook') - return + continue commands = [interpolate_context(description, command, context) for command in commands] @@ -134,7 +134,6 @@ class Before_after_hooks: Example use as a context manager: - with borgmatic.hooks.command.Before_after_hooks( command_hooks=config.get('commands'), before_after='do_stuff', diff --git a/tests/integration/config/test_generate.py b/tests/integration/config/test_generate.py index 2db4823a..f58d6002 100644 --- a/tests/integration/config/test_generate.py +++ b/tests/integration/config/test_generate.py @@ -16,6 +16,116 @@ def test_insert_newline_before_comment_does_not_raise(): module.insert_newline_before_comment(config, field_name) +def test_schema_to_sample_configuration_comments_out_non_default_options(): + schema = { + 'type': 'object', + 'properties': dict( + [ + ('field1', {'example': 'Example 1'}), + ('field2', {'example': 'Example 2'}), + ('source_directories', {'example': 'Example 3'}), + ] + ), + } + + config = module.schema_to_sample_configuration(schema) + + assert config == dict( + [ + ('field1', 'Example 1'), + ('field2', 'Example 2'), + ('source_directories', 'Example 3'), + ] + ) + assert 'COMMENT_OUT' in config.ca.items['field1'][1][-1]._value + assert 'COMMENT_OUT' in config.ca.items['field2'][1][-1]._value + assert 'source_directories' not in config.ca.items + + +def test_schema_to_sample_configuration_comments_out_non_source_config_options(): + schema = { + 'type': 'object', + 'properties': dict( + [ + ('field1', {'example': 'Example 1'}), + ('field2', {'example': 'Example 2'}), + ('field3', {'example': 'Example 3'}), + ] + ), + } + source_config = {'field3': 'value'} + + config = module.schema_to_sample_configuration(schema, source_config) + + assert config == dict( + [ + ('field1', 'Example 1'), + ('field2', 'Example 2'), + ('field3', 'Example 3'), + ] + ) + assert 'COMMENT_OUT' in config.ca.items['field1'][1][-1]._value + assert 'COMMENT_OUT' in config.ca.items['field2'][1][-1]._value + assert 'field3' not in config.ca.items + + +def test_schema_to_sample_configuration_comments_out_non_default_options_in_sequence_of_maps(): + schema = { + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': dict( + [ + ('field1', {'example': 'Example 1'}), + ('field2', {'example': 'Example 2'}), + ('source_directories', {'example': 'Example 3'}), + ] + ), + }, + } + + config = module.schema_to_sample_configuration(schema) + + assert config == [ + dict( + [('field1', 'Example 1'), ('field2', 'Example 2'), ('source_directories', 'Example 3')] + ) + ] + + # The first field in a sequence does not get commented. + assert 'field1' not in config[0].ca.items + assert 'COMMENT_OUT' in config[0].ca.items['field2'][1][-1]._value + assert 'source_directories' not in config[0].ca.items + + +def test_schema_to_sample_configuration_comments_out_non_source_config_options_in_sequence_of_maps(): + schema = { + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': dict( + [ + ('field1', {'example': 'Example 1'}), + ('field2', {'example': 'Example 2'}), + ('field3', {'example': 'Example 3'}), + ] + ), + }, + } + source_config = [{'field3': 'value'}] + + config = module.schema_to_sample_configuration(schema, source_config) + + assert config == [ + dict([('field1', 'Example 1'), ('field2', 'Example 2'), ('field3', 'Example 3')]) + ] + + # The first field in a sequence does not get commented. + assert 'field1' not in config[0].ca.items + assert 'COMMENT_OUT' in config[0].ca.items['field2'][1][-1]._value + assert 'field3' not in config[0].ca.items + + def test_comment_out_line_skips_blank_line(): line = ' \n' diff --git a/tests/unit/config/test_generate.py b/tests/unit/config/test_generate.py index 9b880df6..a10c422d 100644 --- a/tests/unit/config/test_generate.py +++ b/tests/unit/config/test_generate.py @@ -1,5 +1,3 @@ -from collections import OrderedDict - import pytest from flexmock import flexmock @@ -9,7 +7,7 @@ from borgmatic.config import generate as module def test_get_properties_with_simple_object(): schema = { 'type': 'object', - 'properties': OrderedDict( + 'properties': dict( [ ('field1', {'example': 'Example'}), ] @@ -24,7 +22,7 @@ def test_get_properties_merges_one_of_list_properties(): 'type': 'object', 'oneOf': [ { - 'properties': OrderedDict( + 'properties': dict( [ ('field1', {'example': 'Example 1'}), ('field2', {'example': 'Example 2'}), @@ -32,7 +30,7 @@ def test_get_properties_merges_one_of_list_properties(): ), }, { - 'properties': OrderedDict( + 'properties': dict( [ ('field2', {'example': 'Example 2'}), ('field3', {'example': 'Example 3'}), @@ -50,7 +48,7 @@ def test_get_properties_merges_one_of_list_properties(): def test_schema_to_sample_configuration_generates_config_map_with_examples(): schema = { 'type': 'object', - 'properties': OrderedDict( + 'properties': dict( [ ('field1', {'example': 'Example 1'}), ('field2', {'example': 'Example 2'}), @@ -59,12 +57,12 @@ def test_schema_to_sample_configuration_generates_config_map_with_examples(): ), } flexmock(module).should_receive('get_properties').and_return(schema['properties']) - flexmock(module.ruamel.yaml.comments).should_receive('CommentedMap').replace_with(OrderedDict) + flexmock(module.ruamel.yaml.comments).should_receive('CommentedMap').replace_with(dict) flexmock(module).should_receive('add_comments_to_configuration_object') config = module.schema_to_sample_configuration(schema) - assert config == OrderedDict( + assert config == dict( [ ('field1', 'Example 1'), ('field2', 'Example 2'), @@ -88,7 +86,7 @@ def test_schema_to_sample_configuration_generates_config_sequence_of_maps_with_e 'type': 'array', 'items': { 'type': 'object', - 'properties': OrderedDict( + 'properties': dict( [('field1', {'example': 'Example 1'}), ('field2', {'example': 'Example 2'})] ), }, @@ -100,7 +98,7 @@ def test_schema_to_sample_configuration_generates_config_sequence_of_maps_with_e config = module.schema_to_sample_configuration(schema) - assert config == [OrderedDict([('field1', 'Example 1'), ('field2', 'Example 2')])] + assert config == [dict([('field1', 'Example 1'), ('field2', 'Example 2')])] def test_schema_to_sample_configuration_generates_config_sequence_of_maps_with_multiple_types(): @@ -108,7 +106,7 @@ def test_schema_to_sample_configuration_generates_config_sequence_of_maps_with_m 'type': 'array', 'items': { 'type': ['object', 'null'], - 'properties': OrderedDict( + 'properties': dict( [('field1', {'example': 'Example 1'}), ('field2', {'example': 'Example 2'})] ), }, @@ -120,7 +118,7 @@ def test_schema_to_sample_configuration_generates_config_sequence_of_maps_with_m config = module.schema_to_sample_configuration(schema) - assert config == [OrderedDict([('field1', 'Example 1'), ('field2', 'Example 2')])] + assert config == [dict([('field1', 'Example 1'), ('field2', 'Example 2')])] def test_schema_to_sample_configuration_with_unsupported_schema_raises(): diff --git a/tests/unit/config/test_normalize.py b/tests/unit/config/test_normalize.py index edef6695..abd7e54d 100644 --- a/tests/unit/config/test_normalize.py +++ b/tests/unit/config/test_normalize.py @@ -123,6 +123,114 @@ def test_normalize_sections_with_only_scalar_raises(): module.normalize_sections('test.yaml', config) +@pytest.mark.parametrize( + 'config,expected_config,produces_logs', + ( + ( + {'before_actions': ['foo', 'bar'], 'after_actions': ['baz']}, + { + 'commands': [ + {'before': 'repository', 'run': ['foo', 'bar']}, + {'after': 'repository', 'run': ['baz']}, + ] + }, + True, + ), + ( + {'before_backup': ['foo', 'bar'], 'after_backup': ['baz']}, + { + 'commands': [ + {'before': 'action', 'when': ['create'], 'run': ['foo', 'bar']}, + {'after': 'action', 'when': ['create'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'before_prune': ['foo', 'bar'], 'after_prune': ['baz']}, + { + 'commands': [ + {'before': 'action', 'when': ['prune'], 'run': ['foo', 'bar']}, + {'after': 'action', 'when': ['prune'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'before_compact': ['foo', 'bar'], 'after_compact': ['baz']}, + { + 'commands': [ + {'before': 'action', 'when': ['compact'], 'run': ['foo', 'bar']}, + {'after': 'action', 'when': ['compact'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'before_check': ['foo', 'bar'], 'after_check': ['baz']}, + { + 'commands': [ + {'before': 'action', 'when': ['check'], 'run': ['foo', 'bar']}, + {'after': 'action', 'when': ['check'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'before_extract': ['foo', 'bar'], 'after_extract': ['baz']}, + { + 'commands': [ + {'before': 'action', 'when': ['extract'], 'run': ['foo', 'bar']}, + {'after': 'action', 'when': ['extract'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'on_error': ['foo', 'bar']}, + { + 'commands': [ + { + 'after': 'error', + 'when': ['create', 'prune', 'compact', 'check'], + 'run': ['foo', 'bar'], + }, + ] + }, + True, + ), + ( + {'before_everything': ['foo', 'bar'], 'after_everything': ['baz']}, + { + 'commands': [ + {'before': 'everything', 'when': ['create'], 'run': ['foo', 'bar']}, + {'after': 'everything', 'when': ['create'], 'run': ['baz']}, + ] + }, + True, + ), + ( + {'other': 'options', 'unrelated_to': 'commands'}, + {'other': 'options', 'unrelated_to': 'commands'}, + False, + ), + ), +) +def test_normalize_commands_moves_individual_command_hooks_to_unified_commands( + config, expected_config, produces_logs +): + flexmock(module).should_receive('make_command_hook_deprecation_log').and_return(flexmock()) + + logs = module.normalize_commands('test.yaml', config) + + assert config == expected_config + + if produces_logs: + assert logs + else: + assert logs == [] + + @pytest.mark.parametrize( 'config,expected_config,produces_logs', ( @@ -262,6 +370,7 @@ def test_normalize_applies_hard_coded_normalization_to_config( config, expected_config, produces_logs ): flexmock(module).should_receive('normalize_sections').and_return([]) + flexmock(module).should_receive('normalize_commands').and_return([]) logs = module.normalize('test.yaml', config) expected_config.setdefault('bootstrap', {}) @@ -276,6 +385,7 @@ def test_normalize_applies_hard_coded_normalization_to_config( def test_normalize_config_with_borgmatic_source_directory_warns(): flexmock(module).should_receive('normalize_sections').and_return([]) + flexmock(module).should_receive('normalize_commands').and_return([]) logs = module.normalize('test.yaml', {'borgmatic_source_directory': '~/.borgmatic'}) diff --git a/tests/unit/config/test_validate.py b/tests/unit/config/test_validate.py index 5bc08039..8490bd07 100644 --- a/tests/unit/config/test_validate.py +++ b/tests/unit/config/test_validate.py @@ -202,6 +202,15 @@ def test_guard_configuration_contains_repository_does_not_raise_when_repository_ ) +def test_guard_configuration_contains_repository_does_not_raise_when_repository_is_none(): + flexmock(module).should_receive('repositories_match').never() + + module.guard_configuration_contains_repository( + repository=None, + configurations={'config.yaml': {'repositories': [{'path': 'foo/bar', 'label': 'repo'}]}}, + ) + + def test_guard_configuration_contains_repository_errors_when_repository_does_not_match(): flexmock(module).should_receive('repositories_match').and_return(False) diff --git a/tests/unit/hooks/test_command.py b/tests/unit/hooks/test_command.py index cd077e39..b832d3dd 100644 --- a/tests/unit/hooks/test_command.py +++ b/tests/unit/hooks/test_command.py @@ -1,6 +1,7 @@ import logging import subprocess +import pytest from flexmock import flexmock from borgmatic.hooks import command as module @@ -48,6 +49,245 @@ def test_make_environment_with_pyinstaller_and_LD_LIBRARY_PATH_ORIG_copies_it_in ) == {'LD_LIBRARY_PATH_ORIG': '/lib/lib/lib', 'LD_LIBRARY_PATH': '/lib/lib/lib'} +@pytest.mark.parametrize( + 'hooks,filters,expected_hooks', + ( + ( + ( + { + 'before': 'action', + 'run': ['foo'], + }, + { + 'after': 'action', + 'run': ['bar'], + }, + { + 'before': 'repository', + 'run': ['baz'], + }, + ), + {}, + ( + { + 'before': 'action', + 'run': ['foo'], + }, + { + 'after': 'action', + 'run': ['bar'], + }, + { + 'before': 'repository', + 'run': ['baz'], + }, + ), + ), + ( + ( + { + 'before': 'action', + 'run': ['foo'], + }, + { + 'after': 'action', + 'run': ['bar'], + }, + { + 'before': 'repository', + 'run': ['baz'], + }, + ), + { + 'before': 'action', + }, + ( + { + 'before': 'action', + 'run': ['foo'], + }, + ), + ), + ( + ( + { + 'after': 'action', + 'run': ['foo'], + }, + { + 'before': 'action', + 'run': ['bar'], + }, + { + 'after': 'repository', + 'run': ['baz'], + }, + ), + { + 'after': 'action', + }, + ( + { + 'after': 'action', + 'run': ['foo'], + }, + ), + ), + ( + ( + { + 'before': 'dump_data_sources', + 'hooks': ['postgresql'], + 'run': ['foo'], + }, + { + 'before': 'dump_data_sources', + 'hooks': ['lvm'], + 'run': ['bar'], + }, + { + 'after': 'dump_data_sources', + 'hooks': ['lvm'], + 'run': ['baz'], + }, + ), + { + 'before': 'dump_data_sources', + 'hook_name': 'lvm', + }, + ( + { + 'before': 'dump_data_sources', + 'hooks': ['lvm'], + 'run': ['bar'], + }, + ), + ), + ( + ( + { + 'before': 'dump_data_sources', + 'run': ['foo'], + }, + { + 'before': 'dump_data_sources', + 'run': ['bar'], + }, + { + 'after': 'dump_data_sources', + 'run': ['baz'], + }, + ), + { + 'before': 'dump_data_sources', + 'hook_name': 'lvm', + }, + ( + { + 'before': 'dump_data_sources', + 'run': ['foo'], + }, + { + 'before': 'dump_data_sources', + 'run': ['bar'], + }, + ), + ), + ( + ( + { + 'before': 'dump_data_sources', + 'hooks': ['postgresql', 'zfs', 'lvm'], + 'run': ['foo'], + }, + ), + { + 'before': 'dump_data_sources', + 'hook_name': 'lvm', + }, + ( + { + 'before': 'dump_data_sources', + 'hooks': ['postgresql', 'zfs', 'lvm'], + 'run': ['foo'], + }, + ), + ), + ( + ( + { + 'before': 'action', + 'when': ['create'], + 'run': ['foo'], + }, + { + 'before': 'action', + 'when': ['prune'], + 'run': ['bar'], + }, + { + 'before': 'action', + 'when': ['compact'], + 'run': ['baz'], + }, + ), + { + 'before': 'action', + 'action_names': ['create', 'compact', 'extract'], + }, + ( + { + 'before': 'action', + 'when': ['create'], + 'run': ['foo'], + }, + { + 'before': 'action', + 'when': ['compact'], + 'run': ['baz'], + }, + ), + ), + ( + ( + { + 'before': 'action', + 'run': ['foo'], + }, + { + 'before': 'action', + 'run': ['bar'], + }, + { + 'before': 'action', + 'run': ['baz'], + }, + ), + { + 'before': 'action', + 'action_names': ['create', 'compact', 'extract'], + }, + ( + { + 'before': 'action', + 'run': ['foo'], + }, + { + 'before': 'action', + 'run': ['bar'], + }, + { + 'before': 'action', + 'run': ['baz'], + }, + ), + ), + ), +) +def test_filter_hooks(hooks, filters, expected_hooks): + assert module.filter_hooks(hooks, **filters) == expected_hooks + + LOGGING_ANSWER = flexmock() @@ -126,6 +366,79 @@ def test_execute_hooks_with_error_logs_as_error(): module.execute_hooks([{'after': 'error', 'run': ['foo']}], umask=None, dry_run=False) +def test_execute_hooks_with_before_or_after_raises(): + flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels') + flexmock(module.logging).ANSWER = LOGGING_ANSWER + flexmock(module).should_receive('interpolate_context').never() + flexmock(module).should_receive('make_environment').never() + flexmock(module.borgmatic.execute).should_receive('execute_command').never() + + with pytest.raises(ValueError): + module.execute_hooks( + [ + {'erstwhile': 'create', 'run': ['foo']}, + {'erstwhile': 'create', 'run': ['bar', 'baz']}, + ], + umask=None, + dry_run=False, + ) + + +def test_execute_hooks_without_commands_to_run_does_not_raise(): + flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels') + flexmock(module.logging).ANSWER = LOGGING_ANSWER + flexmock(module).should_receive('interpolate_context').replace_with( + lambda hook_description, command, context: command + ) + flexmock(module).should_receive('make_environment').and_return({}) + + for command in ('foo', 'bar'): + flexmock(module.borgmatic.execute).should_receive('execute_command').with_args( + [command], + output_log_level=LOGGING_ANSWER, + shell=True, + environment={}, + ).once() + + module.execute_hooks( + [{'before': 'create', 'run': []}, {'before': 'create', 'run': ['foo', 'bar']}], + umask=None, + dry_run=False, + ) + + +def test_before_after_hooks_calls_command_hooks(): + commands = [ + {'before': 'repository', 'run': ['foo', 'bar']}, + {'after': 'repository', 'run': ['baz']}, + ] + flexmock(module).should_receive('filter_hooks').with_args( + commands, + before='action', + hook_name='myhook', + action_names=['create'], + ).and_return(flexmock()).once() + flexmock(module).should_receive('filter_hooks').with_args( + commands, + after='action', + hook_name='myhook', + action_names=['create'], + ).and_return(flexmock()).once() + flexmock(module).should_receive('execute_hooks').twice() + + with module.Before_after_hooks( + command_hooks=commands, + before_after='action', + umask=1234, + dry_run=False, + hook_name='myhook', + action_names=['create'], + context1='stuff', + context2='such', + ): + pass + + def test_considered_soft_failure_treats_soft_fail_exit_code_as_soft_fail(): error = subprocess.CalledProcessError(module.SOFT_FAIL_EXIT_CODE, 'try again')