Expand source directories when checking for existence. #683
|
@ -314,7 +314,7 @@ def check_all_source_directories_exist(source_directories):
|
||||||
missing_directories = [
|
missing_directories = [
|
||||||
source_directory
|
source_directory
|
||||||
for source_directory in source_directories
|
for source_directory in source_directories
|
||||||
if not os.path.exists(source_directory)
|
if not all([os.path.exists(directory) for directory in expand_directory(source_directory)])
|
||||||
holocronweaver marked this conversation as resolved
Outdated
|
|||||||
]
|
]
|
||||||
if missing_directories:
|
if missing_directories:
|
||||||
raise ValueError(f"Source directories do not exist: {', '.join(missing_directories)}")
|
raise ValueError(f"Source directories do not exist: {', '.join(missing_directories)}")
|
||||||
|
|
|
@ -2565,3 +2565,18 @@ def test_create_archive_with_non_existent_directory_and_source_directories_must_
|
||||||
storage_config={},
|
storage_config={},
|
||||||
local_borg_version='1.2.3',
|
local_borg_version='1.2.3',
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_check_all_source_directories_exist_with_glob_and_tilde_directories():
|
||||||
holocronweaver marked this conversation as resolved
Outdated
witten
commented
The bad news: You've written an integration test! Specifically, integrating the two units The good news: I think the only test you need to write is much smaller and simpler than this one. IMO all you need to unit test is Let me know if you have any questions. And thank you for taking this on! The bad news: You've written an integration test! Specifically, integrating the two units `create_archive` and `check_all_source_directories_exist` along with testing some of their respective functionality. (Some might argue that they are part of the same unit as one of them is "just" a utility function, but personally I've never subscribed to that view.)
The good news: I think the only test you need to write is much smaller and simpler than this one. IMO all you need to unit test is `check_all_source_directories_exist()`. I don't see an existing test for that(!), so don't feel obliged to test all the cases; just a test for the code path you added would be fine.
Let me know if you have any questions. And thank you for taking this on!
holocronweaver
commented
Will switch from integ to unit test, which will definitely be simpler and more pointed. I was trying to copy the style of the only test I saw using Will switch from integ to unit test, which will definitely be simpler and more pointed. I was trying to copy the style of the only test I saw using `source_directories_must_exist`, without reading enough other tests to notice not all were integ tests.
|
|||||||
|
flexmock(module).should_receive('expand_directory').with_args('foo*').and_return(
|
||||||
|
('foo', 'food')
|
||||||
|
)
|
||||||
|
flexmock(module).should_receive('expand_directory').with_args('~/bar').and_return(
|
||||||
|
('/root/bar',)
|
||||||
|
)
|
||||||
|
flexmock(module.os.path).should_receive('exists').and_return(False)
|
||||||
|
flexmock(module.os.path).should_receive('exists').with_args('foo').and_return(True)
|
||||||
|
flexmock(module.os.path).should_receive('exists').with_args('food').and_return(True)
|
||||||
|
flexmock(module.os.path).should_receive('exists').with_args('/root/bar').and_return(True)
|
||||||
|
|
||||||
|
module.check_all_source_directories_exist(['foo*', '~/bar'])
|
||||||
|
|
Nice! This looks like it'd do it. FYI I generally don't use
map()
in this code base when a list/tuple comprehension will work, just because I findmap()
a tiny bit less readable even if it's more succinct. But it's certainly not a hard-and-fast rule, so feel free to keep this as-is.Fair enough! Will switch to list comprehension to better fit existing code style. Agree it is more readable, which is the common case so makes sense to optimize style for it.