tests: replace spy() with pytest-mock spy #3208

fabiosantoscode · 2020-01-21T14:49:37Z

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

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in 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 repo.
❌ 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. 🙏

Fixes treeverse#3198

ghost

@fabiosantoscode , thanks!

I'd prefer to handle the conversion separately, for now, the use_mocker method is good enough)

pared · 2020-01-21T17:40:27Z

tests/func/test_add.py

    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):


With mocker spy, there is no need to patch, it is automatically done by mocker, so with statement can be removed

Great to know, thanks!

pared · 2020-01-21T17:40:44Z

tests/func/test_add.py

    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):


pared · 2020-01-21T17:40:54Z

tests/func/test_add.py

+        get_dir_checksum_counter = self.mocker.spy(
+            RemoteLOCAL, "get_dir_checksum"
+        )
        with patch.object(


pared · 2020-01-21T17:41:08Z

tests/func/test_add.py

-        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):


pared · 2020-01-21T17:41:38Z

tests/func/test_add.py


-    unprotect_spy = spy(RemoteLOCAL.unprotect)
+    unprotect_spy = mocker.spy(RemoteLOCAL, "unprotect")
    mocker.patch.object(RemoteLOCAL, "unprotect", unprotect_spy)


pared · 2020-01-21T17:41:54Z

tests/func/test_add.py


-    copy_spy = spy(dvc.cache.local.copy)
+    copy_spy = mocker.spy(dvc.cache.local, "copy")
    mocker.patch.object(dvc.cache.local, "copy", copy_spy)


pared · 2020-01-21T17:42:10Z

tests/func/test_data_cloud.py

+        test_get_file_checksum = self.mocker.spy(
+            RemoteLOCAL, "get_file_checksum"
+        )
        with patch.object(


pared · 2020-01-21T17:42:30Z

tests/func/test_import_url.py

+        remove_outs_call_counter = self.mocker.spy(
+            dvc.stage.Stage, "remove_outs"
+        )
        with patch.object(


pared · 2020-01-21T17:42:45Z

tests/unit/utils/test_fs.py

+        contains_symlink_spy = self.mocker.spy(
+            dvc.utils.fs, "contains_symlink_up_to"
+        )
        with patch.object(


fabiosantoscode · 2020-01-21T20:53:11Z

Removed the useless patch.object(), thanks @pared

pared · 2020-01-22T11:11:10Z

@fabiosantoscode, moving to mocker's spy seems good. But to be honest I don't like that we are mixning pytest and UnitTest, I think that those tests (only those that use spy) should be converted, it is something that we will have to do anyway at some point. Could you do that?

fabiosantoscode · 2020-01-22T13:34:53Z

Sounds good @pared let me get to that.

fabiosantoscode · 2020-01-22T14:07:10Z

@pared there ⛄

pared · 2020-01-22T14:25:33Z

tests/func/test_add.py

 from tests.utils import get_gitignore_content
-from tests.utils import spy
+
+dvc_module = dvc  # to disambiguate between the module and the fixture


maybe import dvc as dvc_module?

pared · 2020-01-22T14:25:47Z

tests/func/test_add.py

-        with patch.object(dvc.remote.local, "file_md5", file_md5_counter):
-            ret = main(["config", "cache.type", "copy"])
-            self.assertEqual(ret, 0)
+class TestDvcWithMocker(TestDvc):


Seems like it can be removed now :)

pared · 2020-01-22T14:26:39Z

tests/func/test_add.py

-            self.assertEqual(file_md5_counter.mock.call_count, 1)
+    ret = main(["add", "foo"])
+    assert ret == 0
+    assert file_md5_counter.mock.call_count == 1


can be simplified to file_md5_counter.call_count, this applies to each use case of spy

@pared It is a direct conversion that is explicit, we could leave it as is with == 1.

Should I change it to not have .mock then? It does seem to do the same thing.

It does, I don't think its crucial, let's leave it as is.

pared · 2020-01-22T14:35:34Z

tests/func/test_data_cloud.py

+    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])


No need to use TEST_REMOTE, you can use some string name, like "upstream"

@pared I think it is out of scope for this change 🙂

pared · 2020-01-22T14:44:45Z

tests/func/test_import_url.py

            self.assertEqual(fd.read(), "content")


 class TestShouldRemoveOutsBeforeImport(TestDvc):


This can also be refactored.
prepare external resource using tmp_path_factory fixture (eg tmp_path_factory.mktemp("external_source") / "file"

I copied this before refactoring, then forgot to remove the original :)

pared · 2020-01-22T14:47:26Z

tests/func/test_import_url.py



+def test_should_remove_outs_before_import(mocker, dvc, tmp_path_factory):
+    import_dir = TmpDir(tmp_path_factory.mktemp("import"))


We have erepo_dir fixture for such things:
with erepo_dir.chdir(): erepo_dir.dvc_gen("foo": "foo content")

Also, this test does not pass for py3.5, need to wrap tmp_path_factory.mktemp in fspath.

Makes sense.

pared · 2020-01-22T14:50:51Z

tests/unit/utils/test_fs.py

        dirname_patch.assert_called_once()

    @patch.object(System, "is_symlink", return_value=False)
    def test_should_call_recursive_on_no_condition_matched(self, _):


This test can also be refactored. Just extract it to standalone test, not part of TestContainsLink class.

fabiosantoscode · 2020-01-22T17:32:23Z

I've fixed most comments @pared and @efiop except this one: #3208 (comment) about which I'm not clear on what to do.

pared

Good job @fabiosantoscode! LGTM

tests: replace spy() with pytest-mock spy

5dfcda5

Fixes treeverse#3198

fabiosantoscode requested a review from pared January 21, 2020 14:49

ghost approved these changes Jan 21, 2020

View reviewed changes

pared suggested changes Jan 21, 2020

View reviewed changes

don't use patch.object()

bdf665a

ghost requested a review from pared January 22, 2020 07:02

convert tests to pytest function tests

e41aa00

pared suggested changes Jan 22, 2020

View reviewed changes

address comments

fc92266

efiop approved these changes Jan 22, 2020

View reviewed changes

pared approved these changes Jan 23, 2020

View reviewed changes

efiop merged commit 4903996 into treeverse:master Jan 23, 2020

weekly-digest bot mentioned this pull request Jan 26, 2020

Weekly Digest (19 January, 2020 - 26 January, 2020) #3237

Closed

		self.assertEqual(fd.read(), "content")


		class TestShouldRemoveOutsBeforeImport(TestDvc):



		def test_should_remove_outs_before_import(mocker, dvc, tmp_path_factory):
		import_dir = TmpDir(tmp_path_factory.mktemp("import"))

tests: replace spy() with pytest-mock spy #3208

tests: replace spy() with pytest-mock spy #3208

Uh oh!

Conversation

fabiosantoscode commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiosantoscode commented Jan 21, 2020

Uh oh!

pared commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabiosantoscode commented Jan 22, 2020

Uh oh!

fabiosantoscode commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiosantoscode commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

fabiosantoscode commented Jan 21, 2020 •

edited

Loading

pared commented Jan 22, 2020 •

edited

Loading

fabiosantoscode commented Jan 22, 2020 •

edited

Loading