Repository: iterative/dvc
Pull request: tests: replace spy() with pytest-mock spy
After I'd started, @pared pointed out that it would be best to convert the tests to plain functions as opposed to classes, to better utilise the `mocker` fixture. I ended up having to create a bunch of wrappers just to use this fixture. Let me know if you'd like me to go ahead and convert all of the classes that were using spy into plain pytest function tests.

Fixes #3198

-------

* [x] ❗ Have you followed the guidelines in the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) list?

* [x] 📖 Check this box if this PR **does not** require [documentation](https://dvc.org/doc) updates, or if it does **and** you have created a separate PR in [dvc.org](https://github.com/iterative/dvc.org) with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the [dvc.org](https://github.com/iterative/dvc.org) repo.

* [x] ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏



Problem statement:
Remove spy method
In some places, we use `tests/utils/spy` method to mock spied method so that we can observe how particular actions interact with the target method/function. Current implementation:
```
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
```

assumes that we are spying on some class method (by having `self` in wrapper args). This is not always true, and it leads to hiding first argument if we will use it to spy on function (eg `file_md5`) which results in trimmed call args for functions. 

Example:
```
def test_spy(tmp_dir):
    tmp_dir.gen("file", "file content")

    from tests.utils import spy
    import dvc
    checksum_spy = spy(dvc.utils.file_md5)
    checksum_spy("file")
    assert checksum_spy.mock.call_args_list[0].args == ("file",)
```
should pass, while it does not because args tuple is empty.

[EDIT]
As @mroutis  noted, `pytest`'s mocker can `spy`, so it would be good to remove current spy usages and replace them with `mocker.spy`.

Hints from issue discussion:
@pawel,  there's a `mocker.spy` already: https://pypi.org/project/pytest-mock/

maybe we could move to that one?
@mroutis Thanks for pointing that out! Renaming the issue.
Hey there! I'm on this one :)
@fabiosantoscode 
Sure, I just would like to note that most use cases of our spy are in UnitTest style tests, so removal might go along with refactoring said tests to pytest style tests.
@pared 

I've gotten around that with this small TestDvc subclass:

```
class TestDvcWithMocker(TestDvc):
    @pytest.fixture(autouse=True)
    def use_mocker(self, mocker):
        self.mocker = mocker     
```  

Then subclassing it and accessing self.mocker. I didn't want to go ahead and convert the tests because it felt out of scope, although I do agree that they should be converted.

Commit message: tests: replace spy() with pytest-mock spy

Fixes #3198

Patch to review:
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()


Code review comments:
diff_hunk: @@ -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):
path: tests/func/test_add.py
text: @user1:
With mocker spy, there is no need to patch, it is automatically done by mocker, so with statement can be removed

@author:
Great to know, thanks!