Add missing test coverage for new/changed code (#1019).

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.

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',

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'
 

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():

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'})
 

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)
 

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')