From 48441a94189494789f751e75f9e1919b9b700b13 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Sep 2023 23:40:44 -0400 Subject: [PATCH] Lint test/ (not just git/), fix warnings and a bug This expands flake8 linting to include the test suite, and fixes the resulting warnings. Four code changes are especially notable: - Unit tests from which documentation is automatically generated contain a few occurrences of "# @NoEffect". These suppressions are extended to "# noqa: B015 # @NoEffect" so the corresponding suppression is also applied for flake8. This is significant because it actually changes what appears in the code examples in the generated documentation. However, since the "@NoEffect" annotation was considered acceptable, this may be okay too. The resulting examples did not become excessively long. - Expressions like "[c for c in commits_for_file_generator]" appear in some unit tests from which documentation is automatically generated. The simpler form "list(commits_for_file_generator)" can replace them. This changes the examples in the documentation, but for the better, since that form is simpler (and also a better practice in general, thus a better thing to show readers). So I made those substitutions. - In test_repo.TestRepo.test_git_work_tree_env, the code intended to unpatch environment variables after the test was ineffective, instead causing os.environ to refer to an ordinary dict object that does not affect environment variables when written to. This uses unittest.mock.patch.dict instead, so the variables are unpatched and subsequent writes to environment variables in the test process are effective. - In test_submodule.TestSubmodule._do_base_tests, the expression statement "csm.module().head.ref.tracking_branch() is not None" appeared to be intended as an assertion, in spite of having been annoated @NoEffect. This is in light of the context and because, if the goal were only to exercise the function call, the "is not None" part would be superfluous. So I made it an assertion. --- .flake8 | 2 +- .pre-commit-config.yaml | 2 +- test/test_commit.py | 14 +++++++------- test/test_diff.py | 1 - test/test_docs.py | 8 ++++---- test/test_quick_doc.py | 14 ++++++-------- test/test_remote.py | 2 +- test/test_repo.py | 16 +++++++--------- test/test_submodule.py | 9 +++++---- 9 files changed, 32 insertions(+), 36 deletions(-) diff --git a/.flake8 b/.flake8 index 08001ffac..ed5d036bf 100644 --- a/.flake8 +++ b/.flake8 @@ -26,7 +26,7 @@ ignore = E265,E266,E731,E704, D, RST, RST3 -exclude = .tox,.venv,build,dist,doc,git/ext/,test +exclude = .tox,.venv,build,dist,doc,git/ext/ rst-roles = # for flake8-RST-docstrings attr,class,func,meth,mod,obj,ref,term,var # used by sphinx diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4c92656e4..5a34b8af0 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -9,7 +9,7 @@ repos: flake8-comprehensions==3.14.0, flake8-typing-imports==1.14.0, ] - exclude: ^doc|^git/ext/|^test/ + exclude: ^doc|^git/ext/ - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 diff --git a/test/test_commit.py b/test/test_commit.py index f6fb49d50..527aea334 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -277,7 +277,7 @@ def __init__(self, *args, **kwargs): super(Child, self).__init__(*args, **kwargs) child_commits = list(Child.iter_items(self.rorepo, "master", paths=("CHANGES", "AUTHORS"))) - assert type(child_commits[0]) == Child + assert type(child_commits[0]) is Child def test_iter_items(self): # pretty not allowed @@ -525,12 +525,12 @@ def test_trailers(self): # check that trailer stays empty for multiple msg combinations msgs = [ - f"Subject\n", - f"Subject\n\nBody with some\nText\n", - f"Subject\n\nBody with\nText\n\nContinuation but\n doesn't contain colon\n", - f"Subject\n\nBody with\nText\n\nContinuation but\n only contains one :\n", - f"Subject\n\nBody with\nText\n\nKey: Value\nLine without colon\n", - f"Subject\n\nBody with\nText\n\nLine without colon\nKey: Value\n", + "Subject\n", + "Subject\n\nBody with some\nText\n", + "Subject\n\nBody with\nText\n\nContinuation but\n doesn't contain colon\n", + "Subject\n\nBody with\nText\n\nContinuation but\n only contains one :\n", + "Subject\n\nBody with\nText\n\nKey: Value\nLine without colon\n", + "Subject\n\nBody with\nText\n\nLine without colon\nKey: Value\n", ] for msg in msgs: diff --git a/test/test_diff.py b/test/test_diff.py index 9c3888f03..5aa4408bf 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -7,7 +7,6 @@ import ddt import shutil import tempfile -import unittest from git import ( Repo, GitCommandError, diff --git a/test/test_docs.py b/test/test_docs.py index 4c23e9f81..d6b88855f 100644 --- a/test/test_docs.py +++ b/test/test_docs.py @@ -263,9 +263,9 @@ def test_references_and_objects(self, rw_dir): # [8-test_references_and_objects] hc = repo.head.commit hct = hc.tree - hc != hct # @NoEffect - hc != repo.tags[0] # @NoEffect - hc == repo.head.reference.commit # @NoEffect + hc != hct # noqa: B015 # @NoEffect + hc != repo.tags[0] # noqa: B015 # @NoEffect + hc == repo.head.reference.commit # noqa: B015 # @NoEffect # ![8-test_references_and_objects] # [9-test_references_and_objects] @@ -369,7 +369,7 @@ def test_references_and_objects(self, rw_dir): # The index contains all blobs in a flat list assert len(list(index.iter_blobs())) == len([o for o in repo.head.commit.tree.traverse() if o.type == "blob"]) # Access blob objects - for (_path, _stage), entry in index.entries.items(): + for (_path, _stage), _entry in index.entries.items(): pass new_file_path = os.path.join(repo.working_tree_dir, "new-file-name") open(new_file_path, "w").close() diff --git a/test/test_quick_doc.py b/test/test_quick_doc.py index 9dc7b8d2e..342a7f293 100644 --- a/test/test_quick_doc.py +++ b/test/test_quick_doc.py @@ -1,6 +1,3 @@ -import pytest - - from test.lib import TestBase from test.lib.helper import with_rw_directory @@ -25,10 +22,11 @@ def test_init_repo_object(self, path_to_dir): repo = Repo(path_to_dir) # ![2-test_init_repo_object] + del repo # Avoids "assigned to but never used" warning. Doesn't go in the docs. + @with_rw_directory def test_cloned_repo_object(self, local_dir): from git import Repo - import git # code to clone from url # [1-test_cloned_repo_object] @@ -72,7 +70,7 @@ def test_cloned_repo_object(self, local_dir): # [6-test_cloned_repo_object] commits_for_file_generator = repo.iter_commits(all=True, max_count=10, paths=update_file) - commits_for_file = [c for c in commits_for_file_generator] + commits_for_file = list(commits_for_file_generator) commits_for_file # Outputs: [, @@ -136,7 +134,7 @@ def test_cloned_repo_object(self, local_dir): # Compare commit to commit # [11.3-test_cloned_repo_object] - first_commit = [c for c in repo.iter_commits(all=True)][-1] + first_commit = list(repo.iter_commits(all=True))[-1] diffs = repo.head.commit.diff(first_commit) for d in diffs: print(d.a_path) @@ -154,7 +152,7 @@ def test_cloned_repo_object(self, local_dir): # Previous commit tree # [13-test_cloned_repo_object] - prev_commits = [c for c in repo.iter_commits(all=True, max_count=10)] # last 10 commits from all branches + prev_commits = list(repo.iter_commits(all=True, max_count=10)) # last 10 commits from all branches tree = prev_commits[0].tree # ![13-test_cloned_repo_object] @@ -210,7 +208,7 @@ def print_files_from_git(root, level=0): # print previous tree # [18.1-test_cloned_repo_object] - commits_for_file = [c for c in repo.iter_commits(all=True, paths=print_file)] + commits_for_file = list(repo.iter_commits(all=True, paths=print_file)) tree = commits_for_file[-1].tree # gets the first commit tree blob = tree[print_file] diff --git a/test/test_remote.py b/test/test_remote.py index e0dcb4131..7144b2791 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -160,7 +160,7 @@ def _do_test_push_result(self, results, remote): # END error checking # END for each info - if any([info.flags & info.ERROR for info in results]): + if any(info.flags & info.ERROR for info in results): self.assertRaises(GitCommandError, results.raise_if_error) else: # No errors, so this should do nothing diff --git a/test/test_repo.py b/test/test_repo.py index 6432b8c6f..d3bc864cd 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -252,7 +252,8 @@ def test_clone_from_with_path_contains_unicode(self): @with_rw_directory @skip( - "the referenced repository was removed, and one needs to setup a new password controlled repo under the orgs control" + """The referenced repository was removed, and one needs to set up a new + password controlled repo under the org's control.""" ) def test_leaking_password_in_clone_logs(self, rw_dir): password = "fakepassword1234" @@ -758,9 +759,9 @@ def test_blame_complex_revision(self, git): @mock.patch.object(Git, "_call_process") def test_blame_accepts_rev_opts(self, git): - res = self.rorepo.blame("HEAD", "README.md", rev_opts=["-M", "-C", "-C"]) expected_args = ["blame", "HEAD", "-M", "-C", "-C", "--", "README.md"] boilerplate_kwargs = {"p": True, "stdout_as_string": False} + self.rorepo.blame("HEAD", "README.md", rev_opts=["-M", "-C", "-C"]) git.assert_called_once_with(*expected_args, **boilerplate_kwargs) @skipIf( @@ -971,7 +972,7 @@ def _assert_rev_parse(self, name): # history with number ni = 11 history = [obj.parents[0]] - for pn in range(ni): + for _ in range(ni): history.append(history[-1].parents[0]) # END get given amount of commits @@ -1329,6 +1330,7 @@ def test_git_work_tree_env(self, rw_dir): # move .git directory to a subdirectory # set GIT_DIR and GIT_WORK_TREE appropriately # check that repo.working_tree_dir == rw_dir + self.rorepo.clone(join_path_native(rw_dir, "master_repo")) repo_dir = join_path_native(rw_dir, "master_repo") @@ -1338,16 +1340,12 @@ def test_git_work_tree_env(self, rw_dir): os.mkdir(new_subdir) os.rename(old_git_dir, new_git_dir) - oldenv = os.environ.copy() - os.environ["GIT_DIR"] = new_git_dir - os.environ["GIT_WORK_TREE"] = repo_dir + to_patch = {"GIT_DIR": new_git_dir, "GIT_WORK_TREE": repo_dir} - try: + with mock.patch.dict(os.environ, to_patch): r = Repo() self.assertEqual(r.working_tree_dir, repo_dir) self.assertEqual(r.working_dir, repo_dir) - finally: - os.environ = oldenv @with_rw_directory def test_rebasing(self, rw_dir): diff --git a/test/test_submodule.py b/test/test_submodule.py index 88717e220..35ff0d7a8 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -111,7 +111,7 @@ def _do_base_tests(self, rwrepo): # force it to reread its information del smold._url - smold.url == sm.url # @NoEffect + smold.url == sm.url # noqa: B015 # @NoEffect # test config_reader/writer methods sm.config_reader() @@ -248,7 +248,7 @@ def _do_base_tests(self, rwrepo): assert csm.module_exists() # tracking branch once again - csm.module().head.ref.tracking_branch() is not None # @NoEffect + assert csm.module().head.ref.tracking_branch() is not None # this flushed in a sub-submodule assert len(list(rwrepo.iter_submodules())) == 2 @@ -480,8 +480,9 @@ def test_base_bare(self, rwrepo): File "C:\\projects\\gitpython\\git\\cmd.py", line 559, in execute raise GitCommandNotFound(command, err) git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid') - cmdline: git clone -n --shared -v C:\\projects\\gitpython\\.git Users\\appveyor\\AppData\\Local\\Temp\\1\\tmplyp6kr_rnon_bare_test_root_module""", - ) # noqa E501 + cmdline: git clone -n --shared -v C:\\projects\\gitpython\\.git Users\\appveyor\\AppData\\Local\\Temp\\1\\tmplyp6kr_rnon_bare_test_root_module + """, # noqa E501 + ) @with_rw_repo(k_subm_current, bare=False) def test_root_module(self, rwrepo): # Can query everything without problems