From 5dfcda54f923a8994b47ec56c6a9f90eb69685e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 21 Jan 2020 14:47:49 +0000 Subject: [PATCH 1/4] tests: replace spy() with pytest-mock spy Fixes #3198 --- tests/func/test_add.py | 31 +++++++++++++++++++------------ tests/func/test_data_cloud.py | 9 +++++++-- tests/func/test_import_url.py | 9 +++++++-- tests/unit/utils/test_fs.py | 9 +++++++-- tests/utils/__init__.py | 13 ------------- 5 files changed, 40 insertions(+), 31 deletions(-) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 1c885ead76..824fce4afd 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -28,7 +28,6 @@ from dvc.utils.stage import load_stage_file from tests.basic_env import TestDvc from tests.utils import get_gitignore_content -from tests.utils import spy def test_add(tmp_dir, dvc): @@ -245,9 +244,15 @@ def test_dir(self): self.assertEqual(ret, 0) -class TestShouldUpdateStateEntryForFileAfterAdd(TestDvc): +class TestDvcWithMocker(TestDvc): + @pytest.fixture(autouse=True) + def use_mocker(self, mocker): + self.mocker = mocker + + +class TestShouldUpdateStateEntryForFileAfterAdd(TestDvcWithMocker): def test(self): - file_md5_counter = spy(dvc.remote.local.file_md5) + file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") with patch.object(dvc.remote.local, "file_md5", file_md5_counter): ret = main(["config", "cache.type", "copy"]) self.assertEqual(ret, 0) @@ -274,9 +279,9 @@ def test(self): self.assertEqual(file_md5_counter.mock.call_count, 1) -class TestShouldUpdateStateEntryForDirectoryAfterAdd(TestDvc): +class TestShouldUpdateStateEntryForDirectoryAfterAdd(TestDvcWithMocker): def test(self): - file_md5_counter = spy(dvc.remote.local.file_md5) + file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") with patch.object(dvc.remote.local, "file_md5", file_md5_counter): ret = main(["config", "cache.type", "copy"]) @@ -320,11 +325,13 @@ def test(self): self.assertEqual(len(os.listdir(self.dvc.cache.local.cache_dir)), 1) -class TestShouldCollectDirCacheOnlyOnce(TestDvc): +class TestShouldCollectDirCacheOnlyOnce(TestDvcWithMocker): def test(self): from dvc.remote.local import RemoteLOCAL - get_dir_checksum_counter = spy(RemoteLOCAL.get_dir_checksum) + get_dir_checksum_counter = self.mocker.spy( + RemoteLOCAL, "get_dir_checksum" + ) with patch.object( RemoteLOCAL, "get_dir_checksum", get_dir_checksum_counter ): @@ -477,15 +484,15 @@ def test_should_cleanup_after_failed_add(tmp_dir, scm, dvc, repo_template): assert "/bar" not in gitignore_content -class TestShouldNotTrackGitInternalFiles(TestDvc): +class TestShouldNotTrackGitInternalFiles(TestDvcWithMocker): def test(self): - stage_creator_spy = spy(dvc.repo.add._create_stages) + stage_creator_spy = self.mocker.spy(dvc.repo.add, "_create_stages") with patch.object(dvc.repo.add, "_create_stages", stage_creator_spy): ret = main(["add", "-R", self.dvc.root_dir]) self.assertEqual(0, ret) - created_stages_filenames = stage_creator_spy.mock.call_args[0][0] + created_stages_filenames = stage_creator_spy.mock.call_args[0][1] for fname in created_stages_filenames: self.assertNotIn(".git", fname) @@ -572,7 +579,7 @@ def test_readding_dir_should_not_unprotect_all(tmp_dir, dvc, mocker): dvc.add("dir") tmp_dir.gen("dir/new_file", "new_file_content") - unprotect_spy = spy(RemoteLOCAL.unprotect) + unprotect_spy = mocker.spy(RemoteLOCAL, "unprotect") mocker.patch.object(RemoteLOCAL, "unprotect", unprotect_spy) dvc.add("dir") @@ -587,7 +594,7 @@ def test_should_not_checkout_when_adding_cached_copy(tmp_dir, dvc, mocker): shutil.copy("bar", "foo") - copy_spy = spy(dvc.cache.local.copy) + copy_spy = mocker.spy(dvc.cache.local, "copy") mocker.patch.object(dvc.cache.local, "copy", copy_spy) dvc.add("foo") diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 748a7fe631..9abd456aaf 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -28,7 +28,6 @@ from dvc.utils.stage import dump_stage_file from dvc.utils.stage import load_stage_file from tests.basic_env import TestDvc -from tests.utils import spy from tests.remotes import ( Azure, @@ -586,8 +585,14 @@ def test(self): class TestCheckSumRecalculation(TestDvc): + @pytest.fixture(autouse=True) + def use_mocker(self, mocker): + self.mocker = mocker + def test(self): - test_get_file_checksum = spy(RemoteLOCAL.get_file_checksum) + test_get_file_checksum = self.mocker.spy( + RemoteLOCAL, "get_file_checksum" + ) with patch.object( RemoteLOCAL, "get_file_checksum", test_get_file_checksum ): diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index ae59e3aa57..0fbd930d35 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -8,7 +8,6 @@ from dvc.main import main from dvc.utils.fs import makedirs from tests.basic_env import TestDvc -from tests.utils import spy class TestCmdImport(TestDvc): @@ -42,6 +41,10 @@ def test(self): class TestShouldRemoveOutsBeforeImport(TestDvc): + @pytest.fixture(autouse=True) + def use_mocker(self, mocker): + self.mocker = mocker + def setUp(self): super().setUp() tmp_dir = self.mkdtemp() @@ -50,7 +53,9 @@ def setUp(self): fobj.write("content") def test(self): - remove_outs_call_counter = spy(dvc.stage.Stage.remove_outs) + remove_outs_call_counter = self.mocker.spy( + dvc.stage.Stage, "remove_outs" + ) with patch.object( dvc.stage.Stage, "remove_outs", remove_outs_call_counter ): diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index 0c0e19a0be..88166d455d 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -22,7 +22,6 @@ from dvc.utils.fs import makedirs from dvc.utils.fs import walk_files from tests.basic_env import TestDir -from tests.utils import spy class TestMtimeAndSize(TestDir): @@ -45,6 +44,10 @@ def test(self): class TestContainsLink(TestCase): + @pytest.fixture(autouse=True) + def use_mocker(self, mocker): + self.mocker = mocker + def test_should_raise_exception_on_base_path_not_in_path(self): with self.assertRaises(BasePathNotInCheckedPathException): contains_symlink_up_to(os.path.join("foo", "path"), "bar") @@ -72,7 +75,9 @@ def test_should_return_false_on_no_more_dirs_below_path( @patch.object(System, "is_symlink", return_value=False) def test_should_call_recursive_on_no_condition_matched(self, _): - contains_symlink_spy = spy(contains_symlink_up_to) + contains_symlink_spy = self.mocker.spy( + dvc.utils.fs, "contains_symlink_up_to" + ) with patch.object( dvc.utils.fs, "contains_symlink_up_to", contains_symlink_spy ): diff --git a/tests/utils/__init__.py b/tests/utils/__init__.py index a40a91f2cf..9e5448abb3 100644 --- a/tests/utils/__init__.py +++ b/tests/utils/__init__.py @@ -2,22 +2,9 @@ from contextlib import contextmanager from filecmp import dircmp -from mock import MagicMock - from dvc.scm import Git -def spy(method_to_decorate): - mock = MagicMock() - - def wrapper(self, *args, **kwargs): - mock(*args, **kwargs) - return method_to_decorate(self, *args, **kwargs) - - wrapper.mock = mock - return wrapper - - def get_gitignore_content(): with open(Git.GITIGNORE, "r") as gitignore: return gitignore.read().splitlines() From bdf665a34ac89d9b4c55c7f8aba12db228e3d4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 21 Jan 2020 20:52:48 +0000 Subject: [PATCH 2/4] don't use patch.object() --- tests/func/test_add.py | 105 ++++++++++++++++------------------ tests/func/test_data_cloud.py | 26 ++++----- tests/func/test_import_url.py | 8 +-- tests/unit/utils/test_fs.py | 15 ++--- 4 files changed, 67 insertions(+), 87 deletions(-) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 824fce4afd..814eadcc21 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -253,63 +253,62 @@ def use_mocker(self, mocker): class TestShouldUpdateStateEntryForFileAfterAdd(TestDvcWithMocker): def test(self): file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") - with patch.object(dvc.remote.local, "file_md5", file_md5_counter): - ret = main(["config", "cache.type", "copy"]) - self.assertEqual(ret, 0) - ret = main(["add", self.FOO]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["config", "cache.type", "copy"]) + self.assertEqual(ret, 0) + + ret = main(["add", self.FOO]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 1) - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["status"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 1) - ret = main(["run", "-d", self.FOO, "echo foo"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["run", "-d", self.FOO, "echo foo"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 1) - os.rename(self.FOO, self.FOO + ".back") - ret = main(["checkout"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + os.rename(self.FOO, self.FOO + ".back") + ret = main(["checkout"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 1) - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["status"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 1) class TestShouldUpdateStateEntryForDirectoryAfterAdd(TestDvcWithMocker): def test(self): file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") - with patch.object(dvc.remote.local, "file_md5", file_md5_counter): - ret = main(["config", "cache.type", "copy"]) - self.assertEqual(ret, 0) + ret = main(["config", "cache.type", "copy"]) + self.assertEqual(ret, 0) - ret = main(["add", self.DATA_DIR]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["add", self.DATA_DIR]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 3) - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["status"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 3) - ls = "dir" if os.name == "nt" else "ls" - ret = main( - ["run", "-d", self.DATA_DIR, "{} {}".format(ls, self.DATA_DIR)] - ) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ls = "dir" if os.name == "nt" else "ls" + ret = main( + ["run", "-d", self.DATA_DIR, "{} {}".format(ls, self.DATA_DIR)] + ) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 3) - os.rename(self.DATA_DIR, self.DATA_DIR + ".back") - ret = main(["checkout"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + os.rename(self.DATA_DIR, self.DATA_DIR + ".back") + ret = main(["checkout"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 3) - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["status"]) + self.assertEqual(ret, 0) + self.assertEqual(file_md5_counter.mock.call_count, 3) class TestAddCommit(TestDvc): @@ -327,22 +326,17 @@ def test(self): class TestShouldCollectDirCacheOnlyOnce(TestDvcWithMocker): def test(self): - from dvc.remote.local import RemoteLOCAL - get_dir_checksum_counter = self.mocker.spy( RemoteLOCAL, "get_dir_checksum" ) - with patch.object( - RemoteLOCAL, "get_dir_checksum", get_dir_checksum_counter - ): - ret = main(["add", self.DATA_DIR]) - self.assertEqual(0, ret) + ret = main(["add", self.DATA_DIR]) + self.assertEqual(0, ret) - ret = main(["status"]) - self.assertEqual(0, ret) + ret = main(["status"]) + self.assertEqual(0, ret) - ret = main(["status"]) - self.assertEqual(0, ret) + ret = main(["status"]) + self.assertEqual(0, ret) self.assertEqual(1, get_dir_checksum_counter.mock.call_count) @@ -488,9 +482,8 @@ class TestShouldNotTrackGitInternalFiles(TestDvcWithMocker): def test(self): stage_creator_spy = self.mocker.spy(dvc.repo.add, "_create_stages") - with patch.object(dvc.repo.add, "_create_stages", stage_creator_spy): - ret = main(["add", "-R", self.dvc.root_dir]) - self.assertEqual(0, ret) + ret = main(["add", "-R", self.dvc.root_dir]) + self.assertEqual(0, ret) created_stages_filenames = stage_creator_spy.mock.call_args[0][1] for fname in created_stages_filenames: @@ -580,7 +573,6 @@ def test_readding_dir_should_not_unprotect_all(tmp_dir, dvc, mocker): tmp_dir.gen("dir/new_file", "new_file_content") unprotect_spy = mocker.spy(RemoteLOCAL, "unprotect") - mocker.patch.object(RemoteLOCAL, "unprotect", unprotect_spy) dvc.add("dir") assert not unprotect_spy.mock.called @@ -595,7 +587,6 @@ def test_should_not_checkout_when_adding_cached_copy(tmp_dir, dvc, mocker): shutil.copy("bar", "foo") copy_spy = mocker.spy(dvc.cache.local, "copy") - mocker.patch.object(dvc.cache.local, "copy", copy_spy) dvc.add("foo") diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 9abd456aaf..41d7693472 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -6,7 +6,6 @@ from unittest import SkipTest import pytest -from mock import patch from dvc.cache import NamedCache from dvc.config import Config @@ -593,20 +592,17 @@ def test(self): test_get_file_checksum = self.mocker.spy( RemoteLOCAL, "get_file_checksum" ) - with patch.object( - RemoteLOCAL, "get_file_checksum", test_get_file_checksum - ): - url = Local.get_url() - ret = main(["remote", "add", "-d", TEST_REMOTE, url]) - self.assertEqual(ret, 0) - ret = main(["config", "cache.type", "hardlink"]) - self.assertEqual(ret, 0) - ret = main(["add", self.FOO]) - self.assertEqual(ret, 0) - ret = main(["push"]) - self.assertEqual(ret, 0) - ret = main(["run", "-d", self.FOO, "echo foo"]) - self.assertEqual(ret, 0) + url = Local.get_url() + ret = main(["remote", "add", "-d", TEST_REMOTE, url]) + self.assertEqual(ret, 0) + ret = main(["config", "cache.type", "hardlink"]) + self.assertEqual(ret, 0) + ret = main(["add", self.FOO]) + self.assertEqual(ret, 0) + ret = main(["push"]) + self.assertEqual(ret, 0) + ret = main(["run", "-d", self.FOO, "echo foo"]) + self.assertEqual(ret, 0) self.assertEqual(test_get_file_checksum.mock.call_count, 1) diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 0fbd930d35..902e383cab 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -2,7 +2,6 @@ from uuid import uuid4 import pytest -from mock import patch import dvc from dvc.main import main @@ -56,11 +55,8 @@ def test(self): remove_outs_call_counter = self.mocker.spy( dvc.stage.Stage, "remove_outs" ) - with patch.object( - dvc.stage.Stage, "remove_outs", remove_outs_call_counter - ): - ret = main(["import-url", self.external_source]) - self.assertEqual(0, ret) + ret = main(["import-url", self.external_source]) + self.assertEqual(0, ret) self.assertEqual(1, remove_outs_call_counter.mock.call_count) diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index 88166d455d..8863ca77d8 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -78,17 +78,14 @@ def test_should_call_recursive_on_no_condition_matched(self, _): contains_symlink_spy = self.mocker.spy( dvc.utils.fs, "contains_symlink_up_to" ) - with patch.object( - dvc.utils.fs, "contains_symlink_up_to", contains_symlink_spy - ): - # call from full path to match contains_symlink_spy patch path - self.assertFalse( - dvc.utils.fs.contains_symlink_up_to( - os.path.join("foo", "path"), "foo" - ) + # call from full path to match contains_symlink_spy patch path + self.assertFalse( + dvc.utils.fs.contains_symlink_up_to( + os.path.join("foo", "path"), "foo" ) - self.assertEqual(2, contains_symlink_spy.mock.call_count) + ) + self.assertEqual(2, contains_symlink_spy.mock.call_count) @patch.object(System, "is_symlink", return_value=True) def test_should_return_false_when_base_path_is_symlink(self, _): From e41aa00c2023040becc2f5958b2bc7fc10b38267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Wed, 22 Jan 2020 14:04:52 +0000 Subject: [PATCH 3/4] convert tests to pytest function tests --- tests/func/test_add.py | 130 +++++++++++++++++----------------- tests/func/test_data_cloud.py | 36 ++++------ tests/func/test_import_url.py | 14 ++++ 3 files changed, 94 insertions(+), 86 deletions(-) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 814eadcc21..9dd9aff0b8 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -29,6 +29,8 @@ from tests.basic_env import TestDvc from tests.utils import get_gitignore_content +dvc_module = dvc # to disambiguate between the module and the fixture + def test_add(tmp_dir, dvc): stage, = tmp_dir.dvc_gen({"foo": "foo"}) @@ -250,65 +252,66 @@ def use_mocker(self, mocker): self.mocker = mocker -class TestShouldUpdateStateEntryForFileAfterAdd(TestDvcWithMocker): - def test(self): - file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") +def test_should_update_state_entry_for_file_after_add(mocker, dvc, tmp_dir): + file_md5_counter = mocker.spy(dvc_module.remote.local, "file_md5") + tmp_dir.gen("foo", "foo") - ret = main(["config", "cache.type", "copy"]) - self.assertEqual(ret, 0) + ret = main(["config", "cache.type", "copy"]) + assert ret == 0 - ret = main(["add", self.FOO]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["add", "foo"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 1 - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["status"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 1 - ret = main(["run", "-d", self.FOO, "echo foo"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["run", "-d", "foo", "echo foo"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 1 - os.rename(self.FOO, self.FOO + ".back") - ret = main(["checkout"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + os.rename("foo", "foo.back") + ret = main(["checkout"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 1 - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 1) + ret = main(["status"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 1 -class TestShouldUpdateStateEntryForDirectoryAfterAdd(TestDvcWithMocker): - def test(self): - file_md5_counter = self.mocker.spy(dvc.remote.local, "file_md5") +def test_should_update_state_entry_for_directory_after_add( + mocker, dvc, tmp_dir +): + file_md5_counter = mocker.spy(dvc_module.remote.local, "file_md5") - ret = main(["config", "cache.type", "copy"]) - self.assertEqual(ret, 0) + tmp_dir.gen({"data/data": "foo", "data/data_sub/sub_data": "foo"}) - ret = main(["add", self.DATA_DIR]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["config", "cache.type", "copy"]) + assert ret == 0 - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["add", "data"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 3 - ls = "dir" if os.name == "nt" else "ls" - ret = main( - ["run", "-d", self.DATA_DIR, "{} {}".format(ls, self.DATA_DIR)] - ) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ret = main(["status"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 3 - os.rename(self.DATA_DIR, self.DATA_DIR + ".back") - ret = main(["checkout"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + ls = "dir" if os.name == "nt" else "ls" + ret = main(["run", "-d", "data", "{} {}".format(ls, "data")]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 3 - ret = main(["status"]) - self.assertEqual(ret, 0) - self.assertEqual(file_md5_counter.mock.call_count, 3) + os.rename("data", "data" + ".back") + ret = main(["checkout"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 3 + + ret = main(["status"]) + assert ret == 0 + assert file_md5_counter.mock.call_count == 3 class TestAddCommit(TestDvc): @@ -324,20 +327,18 @@ def test(self): self.assertEqual(len(os.listdir(self.dvc.cache.local.cache_dir)), 1) -class TestShouldCollectDirCacheOnlyOnce(TestDvcWithMocker): - def test(self): - get_dir_checksum_counter = self.mocker.spy( - RemoteLOCAL, "get_dir_checksum" - ) - ret = main(["add", self.DATA_DIR]) - self.assertEqual(0, ret) +def test_should_collect_dir_cache_only_once(mocker, tmp_dir, dvc): + tmp_dir.gen({"data/data": "foo"}) + get_dir_checksum_counter = mocker.spy(RemoteLOCAL, "get_dir_checksum") + ret = main(["add", "data"]) + assert ret == 0 - ret = main(["status"]) - self.assertEqual(0, ret) + ret = main(["status"]) + assert ret == 0 - ret = main(["status"]) - self.assertEqual(0, ret) - self.assertEqual(1, get_dir_checksum_counter.mock.call_count) + ret = main(["status"]) + assert ret == 0 + assert get_dir_checksum_counter.mock.call_count == 1 class SymlinkAddTestBase(TestDvc): @@ -478,16 +479,15 @@ def test_should_cleanup_after_failed_add(tmp_dir, scm, dvc, repo_template): assert "/bar" not in gitignore_content -class TestShouldNotTrackGitInternalFiles(TestDvcWithMocker): - def test(self): - stage_creator_spy = self.mocker.spy(dvc.repo.add, "_create_stages") +def test_should_not_track_git_internal_files(mocker, dvc, tmp_dir): + stage_creator_spy = mocker.spy(dvc_module.repo.add, "_create_stages") - ret = main(["add", "-R", self.dvc.root_dir]) - self.assertEqual(0, ret) + ret = main(["add", "-R", dvc.root_dir]) + assert ret == 0 - created_stages_filenames = stage_creator_spy.mock.call_args[0][1] - for fname in created_stages_filenames: - self.assertNotIn(".git", fname) + created_stages_filenames = stage_creator_spy.mock.call_args[0][1] + for fname in created_stages_filenames: + assert ".git" not in fname class TestAddUnprotected(TestDvc): diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 41d7693472..56189d8213 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -583,27 +583,21 @@ def test(self): self._test_recursive_pull() -class TestCheckSumRecalculation(TestDvc): - @pytest.fixture(autouse=True) - def use_mocker(self, mocker): - self.mocker = mocker - - def test(self): - test_get_file_checksum = self.mocker.spy( - RemoteLOCAL, "get_file_checksum" - ) - url = Local.get_url() - ret = main(["remote", "add", "-d", TEST_REMOTE, url]) - self.assertEqual(ret, 0) - ret = main(["config", "cache.type", "hardlink"]) - self.assertEqual(ret, 0) - ret = main(["add", self.FOO]) - self.assertEqual(ret, 0) - ret = main(["push"]) - self.assertEqual(ret, 0) - ret = main(["run", "-d", self.FOO, "echo foo"]) - self.assertEqual(ret, 0) - self.assertEqual(test_get_file_checksum.mock.call_count, 1) +def test_checksum_recalculation(mocker, dvc, tmp_dir): + tmp_dir.gen({"foo": "foo"}) + test_get_file_checksum = mocker.spy(RemoteLOCAL, "get_file_checksum") + url = Local.get_url() + ret = main(["remote", "add", "-d", TEST_REMOTE, url]) + assert ret == 0 + ret = main(["config", "cache.type", "hardlink"]) + assert ret == 0 + ret = main(["add", "foo"]) + assert ret == 0 + ret = main(["push"]) + assert ret == 0 + ret = main(["run", "-d", "foo", "echo foo"]) + assert ret == 0 + assert test_get_file_checksum.mock.call_count == 1 class TestShouldWarnOnNoChecksumInLocalAndRemoteCache(TestDvc): diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 902e383cab..f325e5b14d 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -4,9 +4,12 @@ import pytest import dvc +from dvc.stage import Stage from dvc.main import main from dvc.utils.fs import makedirs +from dvc.compat import fspath from tests.basic_env import TestDvc +from tests.dir_helpers import TmpDir class TestCmdImport(TestDvc): @@ -61,6 +64,17 @@ def test(self): self.assertEqual(1, remove_outs_call_counter.mock.call_count) +def test_should_remove_outs_before_import(mocker, dvc, tmp_path_factory): + import_dir = TmpDir(tmp_path_factory.mktemp("import")) + import_dir.gen({"foo": "foo"}) + + remove_outs_call_counter = mocker.spy(Stage, "remove_outs") + ret = main(["import-url", fspath(import_dir / "foo")]) + + assert ret == 0 + assert remove_outs_call_counter.mock.call_count == 1 + + class TestImportFilename(TestDvc): def setUp(self): super().setUp() From fc922661141c63f67986c7dd8d480cff3a12359c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Wed, 22 Jan 2020 17:31:46 +0000 Subject: [PATCH 4/4] address comments --- tests/func/test_add.py | 10 +--------- tests/func/test_import_url.py | 31 +++---------------------------- tests/unit/utils/test_fs.py | 30 ++++++++++++------------------ 3 files changed, 16 insertions(+), 55 deletions(-) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 9dd9aff0b8..cdd43f9cc3 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -9,7 +9,7 @@ import pytest from mock import patch -import dvc +import dvc as dvc_module from dvc.cache import Cache from dvc.exceptions import DvcException from dvc.exceptions import RecursiveAddingWhileUsingFilename @@ -29,8 +29,6 @@ from tests.basic_env import TestDvc from tests.utils import get_gitignore_content -dvc_module = dvc # to disambiguate between the module and the fixture - def test_add(tmp_dir, dvc): stage, = tmp_dir.dvc_gen({"foo": "foo"}) @@ -246,12 +244,6 @@ def test_dir(self): self.assertEqual(ret, 0) -class TestDvcWithMocker(TestDvc): - @pytest.fixture(autouse=True) - def use_mocker(self, mocker): - self.mocker = mocker - - def test_should_update_state_entry_for_file_after_add(mocker, dvc, tmp_dir): file_md5_counter = mocker.spy(dvc_module.remote.local, "file_md5") tmp_dir.gen("foo", "foo") diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index f325e5b14d..bf042e32b8 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -3,13 +3,11 @@ import pytest -import dvc from dvc.stage import Stage from dvc.main import main from dvc.utils.fs import makedirs from dvc.compat import fspath from tests.basic_env import TestDvc -from tests.dir_helpers import TmpDir class TestCmdImport(TestDvc): @@ -42,34 +40,11 @@ def test(self): self.assertEqual(fd.read(), "content") -class TestShouldRemoveOutsBeforeImport(TestDvc): - @pytest.fixture(autouse=True) - def use_mocker(self, mocker): - self.mocker = mocker - - def setUp(self): - super().setUp() - tmp_dir = self.mkdtemp() - self.external_source = os.path.join(tmp_dir, "file") - with open(self.external_source, "w") as fobj: - fobj.write("content") - - def test(self): - remove_outs_call_counter = self.mocker.spy( - dvc.stage.Stage, "remove_outs" - ) - ret = main(["import-url", self.external_source]) - self.assertEqual(0, ret) - - self.assertEqual(1, remove_outs_call_counter.mock.call_count) - - -def test_should_remove_outs_before_import(mocker, dvc, tmp_path_factory): - import_dir = TmpDir(tmp_path_factory.mktemp("import")) - import_dir.gen({"foo": "foo"}) +def test_should_remove_outs_before_import(mocker, erepo_dir): + erepo_dir.gen({"foo": "foo"}) remove_outs_call_counter = mocker.spy(Stage, "remove_outs") - ret = main(["import-url", fspath(import_dir / "foo")]) + ret = main(["import-url", fspath(erepo_dir / "foo")]) assert ret == 0 assert remove_outs_call_counter.mock.call_count == 1 diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index 8863ca77d8..07c62e0a2b 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -44,10 +44,6 @@ def test(self): class TestContainsLink(TestCase): - @pytest.fixture(autouse=True) - def use_mocker(self, mocker): - self.mocker = mocker - def test_should_raise_exception_on_base_path_not_in_path(self): with self.assertRaises(BasePathNotInCheckedPathException): contains_symlink_up_to(os.path.join("foo", "path"), "bar") @@ -73,20 +69,6 @@ def test_should_return_false_on_no_more_dirs_below_path( ) dirname_patch.assert_called_once() - @patch.object(System, "is_symlink", return_value=False) - def test_should_call_recursive_on_no_condition_matched(self, _): - contains_symlink_spy = self.mocker.spy( - dvc.utils.fs, "contains_symlink_up_to" - ) - - # call from full path to match contains_symlink_spy patch path - self.assertFalse( - dvc.utils.fs.contains_symlink_up_to( - os.path.join("foo", "path"), "foo" - ) - ) - self.assertEqual(2, contains_symlink_spy.mock.call_count) - @patch.object(System, "is_symlink", return_value=True) def test_should_return_false_when_base_path_is_symlink(self, _): base_path = "foo" @@ -111,6 +93,18 @@ def test_path_object_and_str_are_valid_arg_types(self): ) +def test_should_call_recursive_on_no_condition_matched(mocker): + mocker.patch.object(System, "is_symlink", return_value=False) + + contains_symlink_spy = mocker.spy(dvc.utils.fs, "contains_symlink_up_to") + + # call from full path to match contains_symlink_spy patch path + assert not dvc.utils.fs.contains_symlink_up_to( + os.path.join("foo", "path"), "foo" + ) + assert contains_symlink_spy.mock.call_count == 2 + + @pytest.mark.skipif(os.name != "nt", reason="Windows specific") def test_relpath_windows_different_drives(): path1 = os.path.join("A:", os.sep, "some", "path")