From fbb3a8e0bb83ecc39c689f1ce4ff88bd736e71f8 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 8 Jan 2018 22:47:37 -0800 Subject: [PATCH 1/2] dvc fsck unit-tests and refactoring --- dvc/command/fsck.py | 97 ++++++++++++++++++++------------- tests/test_fsck.py | 129 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 36 deletions(-) create mode 100644 tests/test_fsck.py diff --git a/dvc/command/fsck.py b/dvc/command/fsck.py index e729349ef..a3669296d 100644 --- a/dvc/command/fsck.py +++ b/dvc/command/fsck.py @@ -5,23 +5,22 @@ from dvc.command.common.base import CmdBase from dvc.data_cloud import file_md5 -class FileFsckDep(object): - def __init__(self, dvc_file_name, md5, type_name, use_cache): +class FsckFileDep(object): + def __init__(self, dvc_file_name, dep): self.dvc_file_name = dvc_file_name - self.md5 = md5 - self.type_name = type_name - self.use_cache = use_cache - - def checksum_msg(self, hardlink_md5): - stage_error = '!!!' if self.md5 and hardlink_md5 and self.md5 != hardlink_md5 else '' - return '{} {}'.format(self.md5, stage_error) + self.md5 = dep.md5 + self.type_name = type(dep).__name__ + self.use_cache = dep.use_cache @property def is_output(self): return self.type_name == stage.Output.__name__ -class FileFsck(object): +class FsckFile(object): + ERR_STATUS_NO_CACHE_FILE = 'No cache file found' + ERR_STATUS_CHECKSUM_MISMATCH = 'Checksum missmatch' + def __init__(self, dvc_path, full_path, md5, hardlink_md5, state, fsck_deps): self.dvc_path = dvc_path self.full_path = full_path @@ -61,9 +60,9 @@ class FileFsck(object): @property def error_status(self): if self.checksums_mismatch(): - return 'Checksum missmatch' + return self.ERR_STATUS_CHECKSUM_MISMATCH if self.is_data_file() and self.hardlink_md5 is None: - return 'No cache file found' + return self.ERR_STATUS_NO_CACHE_FILE return None def print_info(self): @@ -82,28 +81,36 @@ class FileFsck(object): for fsck_deps in self.fsck_deps: print(u' Stage file: {}'.format(fsck_deps.dvc_file_name)) - print(u' Checksum: {}'.format(fsck_deps.checksum_msg(self.hardlink_md5))) + print(u' Checksum: {}'.format(fsck_deps.md5)) print(u' Type: {}'.format(fsck_deps.type_name)) if fsck_deps.use_cache: print(u' Use cache: {}'.format(str(fsck_deps.use_cache).lower())) pass -class CmdFsck(CmdBase): - def run(self): - directions = self.all_directions() - files_and_stages = self.directions_by_datafile(directions) +class Fsck(object): + def __init__(self, project, targets=None, physical=False, all=False): + self.project = project + self.targets = targets or [] + self.physical = physical + self.all = all - if self.args.targets: + self.files_and_stages = self.directions_by_datafile(self.all_directions()) + self.caches = self.project.cache.find_cache(self.files_and_stages) + + self.fsck_objs = self.dvc_files_to_fsck_objs() + + @property + def is_target_defined(self): + return len(self.targets) > 0 + + def file_list(self, files_and_stages, targets): + if targets: dvc_files = [os.path.relpath(os.path.abspath(f), self.project.root_dir) - for f in self.args.targets] + for f in targets] else: dvc_files = files_and_stages.keys() - - caches = self.project.cache.find_cache(dvc_files) - - self.print_fsck(caches, dvc_files, files_and_stages) - return 0 + return dvc_files def all_directions(self): result = [] @@ -112,28 +119,34 @@ class CmdFsck(CmdBase): result.append((stage, dep)) return result - def create_file_fsck_state(self, dvc_path, caches, files_and_stages): + def create_file_fsck_state(self, dvc_path): full_path = os.path.join(self.project.root_dir, dvc_path) - if self.args.physical: + if self.physical: md5 = file_md5(full_path)[0] else: md5 = None - hardlink_md5 = caches.get(dvc_path) + hardlink_md5 = self.caches.get(dvc_path) state = self.project.state.get(dvc_path) - fsck_deps = [FileFsckDep(stage.dvc_path, dep.md5, type(dep).__name__, dep.use_cache) - for stage, dep in files_and_stages.get(dvc_path, [])] + fsck_deps = [FsckFileDep(stage.dvc_path, dep) + for stage, dep in self.files_and_stages.get(dvc_path, [])] - return FileFsck(dvc_path, full_path, md5, hardlink_md5, state, fsck_deps) + return FsckFile(dvc_path, full_path, md5, hardlink_md5, state, fsck_deps) - def print_fsck(self, caches, dvc_files, files_and_stages): - for file in dvc_files: - file_fsck = self.create_file_fsck_state(file, caches, files_and_stages) + def dvc_files_to_fsck_objs(self): + if not self.is_target_defined or self.all: + files = self.files_and_stages.keys() + else: + files = map(lambda t: os.path.relpath(os.path.abspath(t), self.project.root_dir), + self.targets) - if self.args.all or file_fsck.error_status: - file_fsck.print_info() - pass + fsck_objs = map(lambda file: self.create_file_fsck_state(file), files) + + if not self.is_target_defined and not self.all: + fsck_objs = filter(lambda o: o.error_status is not None, fsck_objs) + + return list(fsck_objs) @staticmethod def directions_by_datafile(directions): @@ -145,3 +158,15 @@ class CmdFsck(CmdBase): result[dep.dvc_path].append((stage, dep)) return result + + +class CmdFsck(CmdBase): + def run(self): + return self.fsck(self.args.targets, self.args.physical, self.args.all) + + def fsck(self, targets, physical, all): + checker = Fsck(self.project, targets, physical, all) + + for file_fsck in checker.fsck_objs: + file_fsck.print_info() + return 0 diff --git a/tests/test_fsck.py b/tests/test_fsck.py new file mode 100644 index 000000000..d20e8761a --- /dev/null +++ b/tests/test_fsck.py @@ -0,0 +1,129 @@ +import os + +from dvc import cli +from tests.basic_env import TestDvc +from dvc.command.fsck import Fsck, FsckFile, CmdFsck + + +class TestFsck(TestDvc): + def no_file_corruptions_test(self): + fsck_objs = Fsck(self.dvc).fsck_objs + self.assertEqual(len(fsck_objs), 0) + + def get_all_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + fsck_objs = Fsck(self.dvc, all=True).fsck_objs + + self.assertEqual(len(fsck_objs), 2) + for obj in fsck_objs: + self.assertIsNone(obj.error_status) + + def get_one_of_many_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + fsck_objs = Fsck(self.dvc, [self.FOO]).fsck_objs + + self.assertEqual(len(fsck_objs), 1) + self.assertEqual(fsck_objs[0].dvc_path, self.FOO) + + def by_file_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + fsck_objs = Fsck(self.dvc, [self.FOO, self.BAR]).fsck_objs + + self.assertEqual(len(fsck_objs), 2) + for obj in fsck_objs: + self.assertIsNone(obj.error_status) + + def defined_target_with_mismatch_test(self): + stage_foo = self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + for out in stage_foo.outs: + if out.dvc_path == self.FOO: + out.md5 = out.md5 + '1234fe' + stage_foo.dump() + + fsck_objs = Fsck(self.dvc, [self.FOO, self.BAR]).fsck_objs + self.assertEqual(len(fsck_objs), 2) + + def checksum_mismatch_test(self): + stage_foo = self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + for out in stage_foo.outs: + if out.dvc_path == self.FOO: + out.md5 = out.md5 + '1234fe' + stage_foo.dump() + + fsck_objs = Fsck(self.dvc).fsck_objs + self.assertEqual(len(fsck_objs), 1) + self.assertEqual(fsck_objs[0].dvc_path, self.FOO) + self.assertEqual(fsck_objs[0].error_status, FsckFile.ERR_STATUS_CHECKSUM_MISMATCH) + + def no_cache_file_test(self): + stage_foo = self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + md5 = '' + for out in stage_foo.outs: + if out.dvc_path == self.FOO: + md5 = out.md5 + + foo_cache_file = os.path.join(self.dvc.cache.cache_dir, md5) + self.assertTrue(os.path.exists(foo_cache_file)) + + os.chmod(foo_cache_file, 0o777) + os.remove(foo_cache_file) + self.assertFalse(os.path.exists(foo_cache_file)) + + fsck_objs = Fsck(self.dvc).fsck_objs + self.assertEqual(len(fsck_objs), 1) + self.assertEqual(fsck_objs[0].dvc_path, self.FOO) + self.assertEqual(fsck_objs[0].error_status, FsckFile.ERR_STATUS_NO_CACHE_FILE) + + def physical_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + os.chmod(self.FOO, 0o777) + os.remove(self.FOO) + with open(self.FOO, 'w') as fd: + fd.write('randome stuff s9dfj') + + fsck_objs = Fsck(self.dvc, physical=True).fsck_objs + + self.assertEqual(len(fsck_objs), 1) + self.assertEqual(fsck_objs[0].dvc_path, self.FOO) + + def by_timestemp_physical_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + os.chmod(self.FOO, 0o777) + os.remove(self.FOO) + with open(self.FOO, 'w') as fd: + fd.write('randome stuff 39re2fwecsb nj') + + fsck_objs = Fsck(self.dvc, physical=False).fsck_objs + + self.assertEqual(len(fsck_objs), 1) + self.assertEqual(fsck_objs[0].dvc_path, self.FOO) + + def end_to_end_test(self): + self.dvc.add(self.FOO) + self.dvc.add(self.BAR) + + os.chmod(self.FOO, 0o777) + os.remove(self.FOO) + + no_data_file = 'baz' + open(no_data_file, 'w').write('asd') + + with open(self.FOO, 'w') as fd: + fd.write('randome stuff s9dfj') + + parser = cli.parse_args(['fsck', self.FOO, self.BAR, no_data_file]) + cmd = CmdFsck(args=parser) + cmd.run() From 9de161a9130e443829dc230950061cb1af2dbaa1 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Tue, 9 Jan 2018 07:40:19 -0800 Subject: [PATCH 2/2] Codereview changes: not more use_cache in Dependency class --- dvc/command/fsck.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/command/fsck.py b/dvc/command/fsck.py index a3669296d..a14614ecc 100644 --- a/dvc/command/fsck.py +++ b/dvc/command/fsck.py @@ -10,7 +10,7 @@ class FsckFileDep(object): self.dvc_file_name = dvc_file_name self.md5 = dep.md5 self.type_name = type(dep).__name__ - self.use_cache = dep.use_cache + self.use_cache = dep.use_cache if hasattr(dep, 'use_cache') else False @property def is_output(self):