[fact] Refactor git module #983

algomaster99 · 2022-01-05T19:26:55Z

Fixes #982

The changes refactor the git module. While restructuring it, I also made the following changes

Renamed Push to PushSpec to avoid confusion between push and Push.
Removed underscore from all methods and prepended them in packages. We know which methods are "public" because they are exposed via __init__.py.
Shift global variables CONCURRENT_TASKS and _EMPTY_REPO_ERROR inside the only function they were used.

- Remove underscore from all methods in git if existed - Push -> PushSpec

algomaster99 · 2022-01-05T19:33:22Z

@slarse I am unsure why the tests are failing. I tried to debug test_tries_all_calls_despite_exceptions and it tries cloning for each spec however it throws an exception in the process. It was somehow suppressing it before. Maybe I would have missed something while refactoring.

slarse

I think I found the problem, you ran into one of the gnarlier pitfalls of mocking in Python. Try my fixes and see if it works.

slarse · 2022-01-14T17:03:12Z

tests/unit_tests/repobee/test_git.py

@@ -200,13 +200,13 @@ async def raise_(pt):
                "Push failed", 128, b"some error", pt.repo_url
            )

-        mocker.patch("_repobee.git._push_async", side_effect=raise_)
+        mocker.patch("_repobee.git.push_async", side_effect=raise_)


Mocks work by monkeypatching the reference to the object. This is the wrong reference to mock, you need to mock the reference that's used in the module you're testing. It took me a long time to debug the first time I ran into this :)

Suggested change

mocker.patch("_repobee.git.push_async", side_effect=raise_)

mocker.patch("_repobee.git._push.push_async", side_effect=raise_)

This is the wrong reference to mock, you need to mock the reference that's used in the module you're testing.

I looked up what is monkey patching and I think I get the gist and use of it. But I don't think I get why this was the wrong reference and we need to provide an exact reference to the method used. Could you please explain "the reference that's used in the module you're testing" again?

slarse · 2022-01-14T17:03:36Z

tests/unit_tests/repobee/test_git.py


        successful_pts, failed_pts = git.push(push_tuples, tries=tries)

        assert not successful_pts
        assert failed_pts == push_tuples
-        git._push_async.assert_has_calls(expected_calls, any_order=True)
+        git.push_async.assert_has_calls(expected_calls, any_order=True)


Suggested change

git.push_async.assert_has_calls(expected_calls, any_order=True)

git._push.push_async.assert_has_calls(expected_calls, any_order=True)

slarse · 2022-01-14T17:05:12Z

tests/unit_tests/repobee/test_git.py

@@ -230,7 +230,7 @@ async def raise_(pt):
            )

        async_push_mock = mocker.patch(
-            "_repobee.git._push_async", side_effect=raise_once
+            "_repobee.git.push_async", side_effect=raise_once


Suggested change

"_repobee.git.push_async", side_effect=raise_once

"_repobee.git._push.push_async", side_effect=raise_once

slarse · 2022-01-14T17:05:23Z

tests/unit_tests/repobee/test_git.py

@@ -307,7 +307,7 @@ async def raise_(spec, *args, **kwargs):
                )

        clone_mock = mocker.patch(
-            "_repobee.git._clone_async", autospec=True, side_effect=raise_
+            "_repobee.git.clone_async", autospec=True, side_effect=raise_


Suggested change

"_repobee.git.clone_async", autospec=True, side_effect=raise_

"_repobee.git._fetch.clone_async", autospec=True, side_effect=raise_

slarse · 2022-01-14T17:06:59Z

src/_repobee/git/__init__.py

+    CloneSpec,
+    CloneStatus,
+    clone,
+    clone_async,


This should not be exposed in the API of this package, as it was private in the module.

Suggested change

clone_async,

I exposed it because it was directly used in the tests. I can make it private.

slarse · 2022-01-14T17:09:59Z

src/_repobee/git/__init__.py

+.. moduleauthor:: Simon Larsén
+"""
+
+from _repobee.git._push import PushSpec, push, push_async  # NOQA


This should not be exposed in the API of this package, as it was private in the module.

Suggested change

from _repobee.git._push import PushSpec, push, push_async # NOQA

from _repobee.git._push import PushSpec, push # NOQA

slarse

Nice!

Now I'd just suggest removing the moduleauthor directive from the module docstrings. It's super inconvenient trying to keep those up-to-date with multiple contributors.

slarse · 2022-01-15T09:26:15Z

src/_repobee/git/_fetch.py

+
+.. moduleauthor:: Simon Larsén


Suggested change

.. moduleauthor:: Simon Larsén

slarse · 2022-01-15T09:26:27Z

src/_repobee/git/_local.py

+
+.. moduleauthor:: Simon Larsén


Suggested change

.. moduleauthor:: Simon Larsén

slarse · 2022-01-15T09:26:39Z

src/_repobee/git/_push.py

+
+.. moduleauthor:: Simon Larsén


Suggested change

.. moduleauthor:: Simon Larsén

slarse · 2022-01-15T09:26:48Z

src/_repobee/git/_util.py

+
+.. moduleauthor:: Simon Larsén


Suggested change

.. moduleauthor:: Simon Larsén

algomaster99 · 2022-01-15T15:37:58Z

@slarse I think we can keep the directive for now since the author has not changed. I would count myself as an author if I had made a fix or added a feature. If there were any authors before, then it makes sense to remove them.

slarse · 2022-01-16T11:05:10Z

@algomaster99 I'm iteratively removing the moduleauthor directive from all modules, so please remove them. They invariably go out of date, for example you didn't add yourself here

slarse

Thanks @algomaster99 , very nice refactoring!

algomaster99 · 2022-01-16T11:15:32Z

I'm iteratively removing the moduleauthor directive from all modules

Okay. I removed the directive from all files suggested above and also git.init.

for example you didn't add yourself here

#980 was also a pure refactoring so I didn't count myself as an author. :)

slarse · 2022-01-16T11:29:43Z

I'm iteratively removing the moduleauthor directive from all modules

Okay. I removed the directive from all files suggested above and also git.init.

for example you didn't add yourself here

#980 was also a pure refactoring so I didn't count myself as an author. :)

I accidentally edited your comment instead of posting a reply...

Anyway, the way I see it, if you edit a file you're an author of that file. I think few devs disagree with that.

algomaster99 added 2 commits January 5, 2022 20:16

Split git module into multiple ones

452db06

Rename methods and classes

42558fb

- Remove underscore from all methods in git if existed - Push -> PushSpec

algomaster99 requested a review from slarse January 5, 2022 19:33

slarse requested changes Jan 14, 2022

View reviewed changes

algomaster99 added 2 commits January 14, 2022 22:34

Use correct reference to method while mocking methods

c6288ef

Add module docstrings

3647d57

algomaster99 requested a review from slarse January 14, 2022 21:56

slarse requested changes Jan 15, 2022

View reviewed changes

Remove moduleauthor directive from git module

4dfc179

algomaster99 requested a review from slarse January 16, 2022 11:11

slarse approved these changes Jan 16, 2022

View reviewed changes

Merge branch 'master' into refactor-git-module

964f9fb

slarse merged commit f317eb9 into repobee:master Jan 16, 2022

algomaster99 deleted the refactor-git-module branch January 16, 2022 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fact] Refactor git module #983

[fact] Refactor git module #983

algomaster99 commented Jan 5, 2022

algomaster99 commented Jan 5, 2022 •

edited

slarse left a comment

slarse Jan 14, 2022

algomaster99 Jan 14, 2022

slarse Jan 14, 2022

slarse Jan 14, 2022

slarse Jan 14, 2022

slarse Jan 14, 2022

algomaster99 Jan 14, 2022

slarse Jan 14, 2022

slarse left a comment

slarse Jan 15, 2022

slarse Jan 15, 2022

slarse Jan 15, 2022

slarse Jan 15, 2022

algomaster99 commented Jan 15, 2022

slarse commented Jan 16, 2022 •

edited

slarse left a comment

algomaster99 commented Jan 16, 2022 •

edited by slarse

slarse commented Jan 16, 2022

	mocker.patch("_repobee.git.push_async", side_effect=raise_)
	mocker.patch("_repobee.git._push.push_async", side_effect=raise_)

	git.push_async.assert_has_calls(expected_calls, any_order=True)
	git._push.push_async.assert_has_calls(expected_calls, any_order=True)

	"_repobee.git.push_async", side_effect=raise_once
	"_repobee.git._push.push_async", side_effect=raise_once

	"_repobee.git.clone_async", autospec=True, side_effect=raise_
	"_repobee.git._fetch.clone_async", autospec=True, side_effect=raise_

	from _repobee.git._push import PushSpec, push, push_async # NOQA
	from _repobee.git._push import PushSpec, push # NOQA

[fact] Refactor git module #983

[fact] Refactor git module #983

Conversation

algomaster99 commented Jan 5, 2022

algomaster99 commented Jan 5, 2022 • edited

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Jan 15, 2022

slarse commented Jan 16, 2022 • edited

slarse left a comment

Choose a reason for hiding this comment

algomaster99 commented Jan 16, 2022 • edited by slarse

slarse commented Jan 16, 2022

algomaster99 commented Jan 5, 2022 •

edited

slarse commented Jan 16, 2022 •

edited

algomaster99 commented Jan 16, 2022 •

edited by slarse