From 355eef186ee966b814b35b0cde07a6cd6ac8014a Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 30 Jan 2025 20:18:03 -0800 Subject: [PATCH] Get existing tests passing again (#961). --- borgmatic/hooks/dispatch.py | 6 +- tests/unit/borg/test_environment.py | 29 +++++- tests/unit/hooks/test_dispatch.py | 18 ++++ tests/unit/test_execute.py | 147 ++++++++++++++++++++++++++-- 4 files changed, 189 insertions(+), 11 deletions(-) diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index d122f75d..26b0832c 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -42,7 +42,11 @@ def call_hook(function_name, config, hook_name, *args, **kwargs): module_name = hook_name.split('_databases')[0] # Probe for a data source or monitoring hook module corresponding to the hook name. - for parent_module in (borgmatic.hooks.credential, borgmatic.hooks.data_source, borgmatic.hooks.monitoring): + for parent_module in ( + borgmatic.hooks.credential, + borgmatic.hooks.data_source, + borgmatic.hooks.monitoring, + ): if module_name not in get_submodule_names(parent_module): continue diff --git a/tests/unit/borg/test_environment.py b/tests/unit/borg/test_environment.py index 30b45746..33b4417e 100644 --- a/tests/unit/borg/test_environment.py +++ b/tests/unit/borg/test_environment.py @@ -3,13 +3,22 @@ from flexmock import flexmock from borgmatic.borg import environment as module -def test_make_environment_with_passcommand_should_set_environment(): +def test_make_environment_with_passcommand_should_call_it_and_set_passphrase_file_descriptor_in_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return('passphrase') + flexmock(module.os).should_receive('pipe').and_return((3, 4)) + flexmock(module.os).should_receive('write') + flexmock(module.os).should_receive('close') + flexmock(module.os).should_receive('set_inheritable') + environment = module.make_environment({'encryption_passcommand': 'command'}) - assert environment.get('BORG_PASSCOMMAND') == 'command' + assert not environment.get('BORG_PASSCOMMAND') + assert environment.get('BORG_PASSPHRASE_FD') == '3' def test_make_environment_with_passphrase_should_set_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'encryption_passphrase': 'pass'}) @@ -17,6 +26,8 @@ def test_make_environment_with_passphrase_should_set_environment(): def test_make_environment_with_ssh_command_should_set_environment(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'ssh_command': 'ssh -C'}) @@ -24,6 +35,8 @@ def test_make_environment_with_ssh_command_should_set_environment(): def test_make_environment_without_configuration_sets_certain_environment_variables(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({}) @@ -36,6 +49,8 @@ def test_make_environment_without_configuration_sets_certain_environment_variabl def test_make_environment_without_configuration_does_not_set_certain_environment_variables_if_already_set(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').with_args( 'BORG_RELOCATED_REPO_ACCESS_IS_OK' ).and_return('yup') @@ -48,6 +63,8 @@ def test_make_environment_without_configuration_does_not_set_certain_environment def test_make_environment_with_relocated_repo_access_true_should_set_environment_yes(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'relocated_repo_access_is_ok': True}) @@ -55,6 +72,8 @@ def test_make_environment_with_relocated_repo_access_true_should_set_environment def test_make_environment_with_relocated_repo_access_false_should_set_environment_no(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'relocated_repo_access_is_ok': False}) @@ -62,6 +81,8 @@ def test_make_environment_with_relocated_repo_access_false_should_set_environmen def test_make_environment_check_i_know_what_i_am_doing_true_should_set_environment_YES(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'check_i_know_what_i_am_doing': True}) @@ -69,6 +90,8 @@ def test_make_environment_check_i_know_what_i_am_doing_true_should_set_environme def test_make_environment_check_i_know_what_i_am_doing_false_should_set_environment_NO(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'check_i_know_what_i_am_doing': False}) @@ -76,6 +99,8 @@ def test_make_environment_check_i_know_what_i_am_doing_false_should_set_environm def test_make_environment_with_integer_variable_value(): + flexmock(module.borgmatic.hooks.dispatch).should_receive('call_hook').and_return(None) + flexmock(module.os).should_receive('pipe').never() flexmock(module.os.environ).should_receive('get').and_return(None) environment = module.make_environment({'borg_files_cache_ttl': 40}) assert environment.get('BORG_FILES_CACHE_TTL') == '40' diff --git a/tests/unit/hooks/test_dispatch.py b/tests/unit/hooks/test_dispatch.py index 80f8c046..8f3f8ecf 100644 --- a/tests/unit/hooks/test_dispatch.py +++ b/tests/unit/hooks/test_dispatch.py @@ -17,6 +17,9 @@ def test_call_hook_invokes_module_function_with_arguments_and_returns_value(): config = {'super_hook': flexmock(), 'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -39,6 +42,9 @@ def test_call_hook_probes_config_with_databases_suffix(): config = {'super_hook_databases': flexmock(), 'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -61,6 +67,9 @@ def test_call_hook_strips_databases_suffix_from_hook_name(): config = {'super_hook_databases': flexmock(), 'other_hook_databases': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -83,6 +92,9 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an config = {'other_hook': flexmock()} expected_return_value = flexmock() test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -104,6 +116,9 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an def test_call_hook_without_corresponding_module_raises(): config = {'super_hook': flexmock(), 'other_hook': flexmock()} test_module = sys.modules[__name__] + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) @@ -121,6 +136,9 @@ def test_call_hook_without_corresponding_module_raises(): def test_call_hook_skips_non_hook_modules(): config = {'not_a_hook': flexmock(), 'other_hook': flexmock()} + flexmock(module).should_receive('get_submodule_names').with_args( + module.borgmatic.hooks.credential + ).and_return(['other_hook']) flexmock(module).should_receive('get_submodule_names').with_args( module.borgmatic.hooks.data_source ).and_return(['other_hook']) diff --git a/tests/unit/test_execute.py b/tests/unit/test_execute.py index 7741526f..8de49912 100644 --- a/tests/unit/test_execute.py +++ b/tests/unit/test_execute.py @@ -185,6 +185,7 @@ def test_execute_command_calls_full_command(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -207,6 +208,7 @@ def test_execute_command_calls_full_command_with_output_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stderr=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -221,7 +223,14 @@ def test_execute_command_calls_full_command_without_capturing_output(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('Popen').with_args( - full_command, stdin=None, stdout=None, stderr=None, shell=False, env=None, cwd=None + full_command, + stdin=None, + stdout=None, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(wait=lambda: 0)).once() flexmock(module).should_receive('interpret_exit_code').and_return(module.Exit_status.SUCCESS) flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) @@ -245,6 +254,7 @@ def test_execute_command_calls_full_command_with_input_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -266,6 +276,7 @@ def test_execute_command_calls_full_command_with_shell(): shell=True, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -287,6 +298,7 @@ def test_execute_command_calls_full_command_with_extra_environment(): shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -308,6 +320,7 @@ def test_execute_command_calls_full_command_with_working_directory(): shell=False, env=None, cwd='/working', + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -317,6 +330,28 @@ def test_execute_command_calls_full_command_with_working_directory(): assert output is None +def test_execute_command_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('Popen').with_args( + full_command, + stdin=None, + stdout=module.subprocess.PIPE, + stderr=module.subprocess.STDOUT, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(stdout=None)).once() + flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) + flexmock(module).should_receive('log_outputs') + + output = module.execute_command(full_command, extra_environment={'BORG_PASSPHRASE_FD': '4'}) + + assert output is None + + def test_execute_command_without_run_to_completion_returns_process(): full_command = ['foo', 'bar'] process = flexmock() @@ -330,6 +365,7 @@ def test_execute_command_without_run_to_completion_returns_process(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(process).once() flexmock(module.borgmatic.logger).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('log_outputs') @@ -343,7 +379,12 @@ def test_execute_command_and_capture_output_returns_stdout(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command) @@ -357,7 +398,12 @@ def test_execute_command_and_capture_output_with_capture_stderr_returns_stderr() flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=module.subprocess.STDOUT, shell=False, env=None, cwd=None + full_command, + stderr=module.subprocess.STDOUT, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command, capture_stderr=True) @@ -372,7 +418,12 @@ def test_execute_command_and_capture_output_returns_output_when_process_error_is flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(1, full_command, err_output)).once() flexmock(module).should_receive('interpret_exit_code').and_return( module.Exit_status.SUCCESS @@ -389,7 +440,12 @@ def test_execute_command_and_capture_output_raises_when_command_errors(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd=None + full_command, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(2, full_command, expected_output)).once() flexmock(module).should_receive('interpret_exit_code').and_return( module.Exit_status.ERROR @@ -405,7 +461,12 @@ def test_execute_command_and_capture_output_returns_output_with_shell(): flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - 'foo bar', stderr=None, shell=True, env=None, cwd=None + 'foo bar', + stderr=None, + shell=True, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output(full_command, shell=True) @@ -424,6 +485,7 @@ def test_execute_command_and_capture_output_returns_output_with_extra_environmen shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output( @@ -439,7 +501,12 @@ def test_execute_command_and_capture_output_returns_output_with_working_director flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( - full_command, stderr=None, shell=False, env=None, cwd='/working' + full_command, + stderr=None, + shell=False, + env=None, + cwd='/working', + close_fds=True, ).and_return(flexmock(decode=lambda: expected_output)).once() output = module.execute_command_and_capture_output( @@ -449,6 +516,29 @@ def test_execute_command_and_capture_output_returns_output_with_working_director assert output == expected_output +def test_execute_command_and_capture_output_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + expected_output = '[]' + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('check_output').with_args( + full_command, + stderr=None, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(decode=lambda: expected_output)).once() + + output = module.execute_command_and_capture_output( + full_command, + shell=False, + extra_environment={'BORG_PASSPHRASE_FD': '4'}, + ) + + assert output == expected_output + + def test_execute_command_with_processes_calls_full_command(): full_command = ['foo', 'bar'] processes = (flexmock(),) @@ -462,6 +552,7 @@ def test_execute_command_with_processes_calls_full_command(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -484,6 +575,7 @@ def test_execute_command_with_processes_returns_output_with_output_log_level_non shell=False, env=None, cwd=None, + close_fds=True, ).and_return(process).once() flexmock(module).should_receive('log_outputs').and_return({process: 'out'}) @@ -506,6 +598,7 @@ def test_execute_command_with_processes_calls_full_command_with_output_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stderr=None)).once() flexmock(module).should_receive('log_outputs') @@ -520,7 +613,14 @@ def test_execute_command_with_processes_calls_full_command_without_capturing_out flexmock(module).should_receive('log_command') flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('Popen').with_args( - full_command, stdin=None, stdout=None, stderr=None, shell=False, env=None, cwd=None + full_command, + stdin=None, + stdout=None, + stderr=None, + shell=False, + env=None, + cwd=None, + close_fds=True, ).and_return(flexmock(wait=lambda: 0)).once() flexmock(module).should_receive('interpret_exit_code').and_return(module.Exit_status.SUCCESS) flexmock(module).should_receive('log_outputs') @@ -546,6 +646,7 @@ def test_execute_command_with_processes_calls_full_command_with_input_file(): shell=False, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -567,6 +668,7 @@ def test_execute_command_with_processes_calls_full_command_with_shell(): shell=True, env=None, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -588,6 +690,7 @@ def test_execute_command_with_processes_calls_full_command_with_extra_environmen shell=False, env={'a': 'b', 'c': 'd'}, cwd=None, + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -611,6 +714,7 @@ def test_execute_command_with_processes_calls_full_command_with_working_director shell=False, env=None, cwd='/working', + close_fds=True, ).and_return(flexmock(stdout=None)).once() flexmock(module).should_receive('log_outputs') @@ -621,6 +725,32 @@ def test_execute_command_with_processes_calls_full_command_with_working_director assert output is None +def test_execute_command_with_processes_with_BORG_PASSPHRASE_FD_leaves_file_descriptors_open(): + full_command = ['foo', 'bar'] + processes = (flexmock(),) + flexmock(module).should_receive('log_command') + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('Popen').with_args( + full_command, + stdin=None, + stdout=module.subprocess.PIPE, + stderr=module.subprocess.STDOUT, + shell=False, + env={'a': 'b', 'BORG_PASSPHRASE_FD': '4'}, + cwd=None, + close_fds=False, + ).and_return(flexmock(stdout=None)).once() + flexmock(module).should_receive('log_outputs') + + output = module.execute_command_with_processes( + full_command, + processes, + extra_environment={'BORG_PASSPHRASE_FD': '4'}, + ) + + assert output is None + + def test_execute_command_with_processes_kills_processes_on_error(): full_command = ['foo', 'bar'] flexmock(module).should_receive('log_command') @@ -637,6 +767,7 @@ def test_execute_command_with_processes_kills_processes_on_error(): shell=False, env=None, cwd=None, + close_fds=True, ).and_raise(subprocess.CalledProcessError(1, full_command, 'error')).once() flexmock(module).should_receive('log_outputs').never()