From 9b85c5bc61410eca8a013547535119f0779c7c42 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Tue, 24 Dec 2024 15:24:09 -0800 Subject: [PATCH] Add missing restore test coverage (#418). --- borgmatic/actions/restore.py | 7 +- borgmatic/commands/arguments.py | 6 +- tests/end-to-end/test_database.py | 10 +- tests/unit/actions/test_restore.py | 269 +++++++++++++++++++++++++---- 4 files changed, 248 insertions(+), 44 deletions(-) diff --git a/borgmatic/actions/restore.py b/borgmatic/actions/restore.py index ba1d9664..9323588f 100644 --- a/borgmatic/actions/restore.py +++ b/borgmatic/actions/restore.py @@ -1,5 +1,4 @@ import collections -import copy import logging import os import pathlib @@ -48,7 +47,7 @@ def dumps_match(first, second): def render_dump_metadata(dump): ''' - Given a Dump instance, make a display string describing it for use in log messges. + Given a Dump instance, make a display string describing it for use in log messages. ''' name = dump.data_source_name if dump.data_source_name != UNSPECIFIED else 'unspecified' hostname = dump.hostname or 'localhost' @@ -97,7 +96,7 @@ def get_configured_data_source(config, restore_dump): if len(matching_dumps) > 1: raise ValueError( - f'Cannot restore data source {render_dump_metadata(restore_dump)} because there are multiple matching configured data sources.' + f'Cannot restore data source {render_dump_metadata(restore_dump)} because there are multiple matching configured data sources' ) return matching_dumps[0] @@ -353,7 +352,7 @@ def get_dumps_to_restore(restore_arguments, dumps_from_archive): if any(dump for dump in requested_dumps if dump.data_source_name == 'all'): dumps_to_restore.update(dumps_from_archive) - # Put any archive dump matching a requested dump in the dumps to restore. + # If any archive dump matches a requested dump, add the archive dump to the dumps to restore. for requested_dump in requested_dumps: if requested_dump.data_source_name == 'all': continue diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index 7e08b726..7bd1d74e 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -1184,16 +1184,16 @@ def make_parsers(): ) restore_group.add_argument( '--original-hostname', - help="The hostname where the dump to restore came from, only necessary if you need to disambiguate dumps", + help='The hostname where the dump to restore came from, only necessary if you need to disambiguate dumps', ) restore_group.add_argument( '--original-port', type=int, - help="The port where the dump to restore came from, only necessary if you need to disambiguate dumps", + help='The port where the dump to restore came from, only necessary if you need to disambiguate dumps', ) restore_group.add_argument( '--hook', - help="The name of the data source hook for the dump to restore, only necessary if you need to disambiguate dumps", + help='The name of the data source hook for the dump to restore, only necessary if you need to disambiguate dumps', ) restore_group.add_argument( '-h', '--help', action='help', help='Show this help message and exit' diff --git a/tests/end-to-end/test_database.py b/tests/end-to-end/test_database.py index 7dba4fd6..a0b9e898 100644 --- a/tests/end-to-end/test_database.py +++ b/tests/end-to-end/test_database.py @@ -25,8 +25,12 @@ def write_configuration( for testing. This includes injecting the given repository path, borgmatic source directory for storing database dumps, dump format (for PostgreSQL), and encryption passphrase. ''' - postgresql_all_format_option = f'format: {postgresql_all_dump_format}' if postgresql_all_dump_format else '' - mariadb_mysql_dump_format_option = f'format: {mariadb_mysql_all_dump_format}' if mariadb_mysql_all_dump_format else '' + postgresql_all_format_option = ( + f'format: {postgresql_all_dump_format}' if postgresql_all_dump_format else '' + ) + mariadb_mysql_dump_format_option = ( + f'format: {mariadb_mysql_all_dump_format}' if mariadb_mysql_all_dump_format else '' + ) config_yaml = f''' source_directories: @@ -95,7 +99,7 @@ sqlite_databases: ( (None, None), ('custom', 'sql'), - ) + ), ) def write_custom_restore_configuration( source_directory, diff --git a/tests/unit/actions/test_restore.py b/tests/unit/actions/test_restore.py index cd064688..0f254780 100644 --- a/tests/unit/actions/test_restore.py +++ b/tests/unit/actions/test_restore.py @@ -1,12 +1,142 @@ -import collections - import pytest from flexmock import flexmock import borgmatic.actions.restore as module -def test_get_configured_data_source_matches_data_source_by_name(): +@pytest.mark.parametrize( + 'first_dump,second_dump,expected_result', + ( + ( + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'foo'), + True, + ), + ( + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'bar'), + False, + ), + ( + module.Dump('postgresql_databases', 'foo'), + module.Dump('mariadb_databases', 'foo'), + False, + ), + (module.Dump('postgresql_databases', 'foo'), module.Dump(module.UNSPECIFIED, 'foo'), True), + (module.Dump('postgresql_databases', 'foo'), module.Dump(module.UNSPECIFIED, 'bar'), False), + ( + module.Dump('postgresql_databases', module.UNSPECIFIED), + module.Dump('postgresql_databases', 'foo'), + True, + ), + ( + module.Dump('postgresql_databases', module.UNSPECIFIED), + module.Dump('mariadb_databases', 'foo'), + False, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost'), + module.Dump('postgresql_databases', 'foo', 'myhost'), + True, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost'), + module.Dump('postgresql_databases', 'foo', 'otherhost'), + False, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost'), + module.Dump('postgresql_databases', 'foo', module.UNSPECIFIED), + True, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost'), + module.Dump('postgresql_databases', 'bar', module.UNSPECIFIED), + False, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost', 1234), + module.Dump('postgresql_databases', 'foo', 'myhost', 1234), + True, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost', 1234), + module.Dump('postgresql_databases', 'foo', 'myhost', 4321), + False, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost', module.UNSPECIFIED), + module.Dump('postgresql_databases', 'foo', 'myhost', 1234), + True, + ), + ( + module.Dump('postgresql_databases', 'foo', 'myhost', module.UNSPECIFIED), + module.Dump('postgresql_databases', 'foo', 'otherhost', 1234), + False, + ), + ( + module.Dump( + module.UNSPECIFIED, module.UNSPECIFIED, module.UNSPECIFIED, module.UNSPECIFIED + ), + module.Dump('postgresql_databases', 'foo', 'myhost', 1234), + True, + ), + ), +) +def test_dumps_match_compares_two_dumps_while_respecting_unspecified_values( + first_dump, second_dump, expected_result +): + assert module.dumps_match(first_dump, second_dump) == expected_result + + +@pytest.mark.parametrize( + 'dump,expected_result', + ( + ( + module.Dump('postgresql_databases', 'foo'), + 'foo@localhost (postgresql_databases)', + ), + ( + module.Dump(module.UNSPECIFIED, 'foo'), + 'foo@localhost', + ), + ( + module.Dump('postgresql_databases', module.UNSPECIFIED), + 'unspecified@localhost (postgresql_databases)', + ), + ( + module.Dump('postgresql_databases', 'foo', 'host'), + 'foo@host (postgresql_databases)', + ), + ( + module.Dump('postgresql_databases', 'foo', module.UNSPECIFIED), + 'foo (postgresql_databases)', + ), + ( + module.Dump('postgresql_databases', 'foo', 'host', 1234), + 'foo@host:1234 (postgresql_databases)', + ), + ( + module.Dump('postgresql_databases', 'foo', module.UNSPECIFIED, 1234), + 'foo@:1234 (postgresql_databases)', + ), + ( + module.Dump('postgresql_databases', 'foo', 'host', module.UNSPECIFIED), + 'foo@host (postgresql_databases)', + ), + ( + module.Dump( + module.UNSPECIFIED, module.UNSPECIFIED, module.UNSPECIFIED, module.UNSPECIFIED + ), + 'unspecified', + ), + ), +) +def test_render_dump_metadata_renders_dump_values_into_string(dump, expected_result): + assert module.render_dump_metadata(dump) == expected_result + + +def test_get_configured_data_source_matches_data_source_with_restore_dump(): flexmock(module).should_receive('dumps_match').and_return(False) flexmock(module).should_receive('dumps_match').with_args( module.Dump('postgresql_databases', 'bar'), @@ -25,21 +155,49 @@ def test_get_configured_data_source_matches_data_source_by_name(): def test_get_configured_data_source_matches_nothing_when_nothing_configured(): flexmock(module).should_receive('dumps_match').and_return(False) - assert module.get_configured_data_source( - config={}, - restore_dump=module.Dump('postgresql_databases', 'quux'), - ) is None + assert ( + module.get_configured_data_source( + config={}, + restore_dump=module.Dump('postgresql_databases', 'quux'), + ) + is None + ) -def test_get_configured_data_source_matches_nothing_when_data_source_name_not_configured(): +def test_get_configured_data_source_matches_nothing_when_restore_dump_does_not_match_configuration(): flexmock(module).should_receive('dumps_match').and_return(False) - assert module.get_configured_data_source( - config={ - 'postgresql_databases': [{'name': 'foo'}], - }, - restore_dump=module.Dump('postgresql_databases', 'quux'), - ) is None + assert ( + module.get_configured_data_source( + config={ + 'postgresql_databases': [{'name': 'foo'}], + }, + restore_dump=module.Dump('postgresql_databases', 'quux'), + ) + is None + ) + + +def test_get_configured_data_source_with_multiple_matching_data_sources_errors(): + flexmock(module).should_receive('dumps_match').and_return(False) + flexmock(module).should_receive('dumps_match').with_args( + module.Dump('postgresql_databases', 'bar'), + module.Dump('postgresql_databases', 'bar'), + ).and_return(True) + flexmock(module).should_receive('render_dump_metadata').and_return('test') + + with pytest.raises(ValueError): + module.get_configured_data_source( + config={ + 'other_databases': [{'name': 'other'}], + 'postgresql_databases': [ + {'name': 'foo'}, + {'name': 'bar'}, + {'name': 'bar', 'format': 'directory'}, + ], + }, + restore_dump=module.Dump('postgresql_databases', 'bar'), + ) def test_strip_path_prefix_from_extracted_dump_destination_renames_first_matching_databases_subdirectory(): @@ -63,6 +221,7 @@ def test_strip_path_prefix_from_extracted_dump_destination_renames_first_matchin def test_restore_single_dump_extracts_and_restores_single_file_dump(): + flexmock(module).should_receive('render_dump_metadata').and_return('test') flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hooks').with_args( 'make_data_source_dump_patterns', object, object, object, object, object ).and_return({'postgresql': flexmock()}) @@ -103,6 +262,7 @@ def test_restore_single_dump_extracts_and_restores_single_file_dump(): def test_restore_single_dump_extracts_and_restores_directory_dump(): + flexmock(module).should_receive('render_dump_metadata').and_return('test') flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hooks').with_args( 'make_data_source_dump_patterns', object, object, object, object, object ).and_return({'postgresql': flexmock()}) @@ -145,6 +305,7 @@ def test_restore_single_dump_extracts_and_restores_directory_dump(): def test_restore_single_dump_with_directory_dump_error_cleans_up_temporary_directory(): + flexmock(module).should_receive('render_dump_metadata').and_return('test') flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hooks').with_args( 'make_data_source_dump_patterns', object, object, object, object, object ).and_return({'postgresql': flexmock()}) @@ -188,6 +349,7 @@ def test_restore_single_dump_with_directory_dump_error_cleans_up_temporary_direc def test_restore_single_dump_with_directory_dump_and_dry_run_skips_directory_move_and_cleanup(): + flexmock(module).should_receive('render_dump_metadata').and_return('test') flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hooks').with_args( 'make_data_source_dump_patterns', object, object, object, object, object ).and_return({'postgresql': flexmock()}) @@ -392,6 +554,7 @@ def test_get_dumps_to_restore_raises_for_requested_dumps_missing_from_archive(): module.Dump('postgresql_databases', 'foo'), } flexmock(module).should_receive('dumps_match').and_return(False) + flexmock(module).should_receive('render_dump_metadata').and_return('test') with pytest.raises(ValueError): module.get_dumps_to_restore( @@ -412,15 +575,18 @@ def test_get_dumps_to_restore_without_requested_dumps_finds_all_archive_dumps(): } flexmock(module).should_receive('dumps_match').and_return(False) - assert module.get_dumps_to_restore( - restore_arguments=flexmock( - hook=None, - data_sources=[], - original_hostname=None, - original_port=None, - ), - dumps_from_archive=dumps_from_archive, - ) == dumps_from_archive + assert ( + module.get_dumps_to_restore( + restore_arguments=flexmock( + hook=None, + data_sources=[], + original_hostname=None, + original_port=None, + ), + dumps_from_archive=dumps_from_archive, + ) + == dumps_from_archive + ) def test_get_dumps_to_restore_with_all_in_requested_dumps_finds_all_archive_dumps(): @@ -438,15 +604,18 @@ def test_get_dumps_to_restore_with_all_in_requested_dumps_finds_all_archive_dump module.Dump('postgresql_databases', 'bar'), ).and_return(True) - assert module.get_dumps_to_restore( - restore_arguments=flexmock( - hook=None, - data_sources=['all'], - original_hostname=None, - original_port=None, - ), - dumps_from_archive=dumps_from_archive, - ) == dumps_from_archive + assert ( + module.get_dumps_to_restore( + restore_arguments=flexmock( + hook=None, + data_sources=['all'], + original_hostname=None, + original_port=None, + ), + dumps_from_archive=dumps_from_archive, + ) + == dumps_from_archive + ) def test_get_dumps_to_restore_with_all_in_requested_dumps_plus_additional_requested_dumps_omits_duplicates(): @@ -478,12 +647,40 @@ def test_get_dumps_to_restore_with_all_in_requested_dumps_plus_additional_reques ) -def test_get_dumps_to_restore_raises_for_all_in_requested_dumps_and_requested_dumps_missing_from_archives(): +def test_get_dumps_to_restore_raises_for_multiple_matching_dumps_in_archive(): flexmock(module).should_receive('dumps_match').and_return(False) flexmock(module).should_receive('dumps_match').with_args( module.Dump(module.UNSPECIFIED, 'foo'), module.Dump('postgresql_databases', 'foo'), ).and_return(True) + flexmock(module).should_receive('dumps_match').with_args( + module.Dump(module.UNSPECIFIED, 'foo'), + module.Dump('mariadb_databases', 'foo'), + ).and_return(True) + flexmock(module).should_receive('render_dump_metadata').and_return('test') + + with pytest.raises(ValueError): + module.get_dumps_to_restore( + restore_arguments=flexmock( + hook=None, + data_sources=['foo'], + original_hostname=None, + original_port=None, + ), + dumps_from_archive={ + module.Dump('postgresql_databases', 'foo'), + module.Dump('mariadb_databases', 'foo'), + }, + ) + + +def test_get_dumps_to_restore_raises_for_all_in_requested_dumps_and_requested_dumps_missing_from_archive(): + flexmock(module).should_receive('dumps_match').and_return(False) + flexmock(module).should_receive('dumps_match').with_args( + module.Dump(module.UNSPECIFIED, 'foo'), + module.Dump('postgresql_databases', 'foo'), + ).and_return(True) + flexmock(module).should_receive('render_dump_metadata').and_return('test') with pytest.raises(ValueError): module.get_dumps_to_restore( @@ -519,9 +716,13 @@ def test_ensure_requested_dumps_restored_with_no_dumps_raises(): def test_ensure_requested_dumps_restored_with_missing_dumps_raises(): + flexmock(module).should_receive('render_dump_metadata').and_return('test') + with pytest.raises(ValueError): module.ensure_requested_dumps_restored( - dumps_to_restore={module.Dump(hook_name='postgresql_databases', data_source_name='foo')}, + dumps_to_restore={ + module.Dump(hook_name='postgresql_databases', data_source_name='foo') + }, dumps_actually_restored={ module.Dump(hook_name='postgresql_databases', data_source_name='bar') },