Skip to content

Commit

Permalink
Revise test suite docstrings and comments
Browse files Browse the repository at this point in the history
In a more limited way than in the git/ code.

Docstring revisions are, in spirit, along the same lines as those
in the git package, but much more conservative, because the tests'
docstrings are not rendered into Sphinx documentation and there is
no current plan to do so (so the tradeoffs are different, with even
a tiny decrease in clarity when read in the code being a reason to
avoid changes that use Sphinx roles more robustly or that would
improve hypothetical rendered documentation), and because the test
suite uses docstrings more sparingly and the existing docstrings
were mostly already clear and easy to read.

This wraps commnts to 88 columns as most comments now are in the
git package, except it avoids doing so when doing so would make
anything even slightly less clear, and where it would require
significant further style or spacing changes for it to remain
obvious (even before reading a comment) what the comment applies
to, and in most portions of tutorial-generating test case code
(where, although clarity would improve when reading the tests, it
might sometimes decrease in the generated documentation).
  • Loading branch information
EliahKagan committed Feb 29, 2024
1 parent 608147e commit 5cf5b60
Show file tree
Hide file tree
Showing 17 changed files with 243 additions and 186 deletions.
13 changes: 8 additions & 5 deletions test/lib/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,17 +346,20 @@ class TestBase(TestCase):
"""Base class providing default functionality to all tests such as:
- Utility functions provided by the TestCase base of the unittest method such as::
self.fail("todo")
self.assertRaises(...)
- Class level repository which is considered read-only as it is shared among
all test cases in your type.
Access it using::
self.rorepo # 'ro' stands for read-only
self.rorepo # 'ro' stands for read-only
The rorepo is in fact your current project's git repo. If you refer to specific
shas for your objects, be sure you choose some that are part of the immutable portion
of the project history (so that tests don't fail for others).
shas for your objects, be sure you choose some that are part of the immutable
portion of the project history (so that tests don't fail for others).
"""

def _small_repo_url(self):
Expand All @@ -383,8 +386,8 @@ def tearDownClass(cls):

def _make_file(self, rela_path, data, repo=None):
"""
Create a file at the given path relative to our repository, filled
with the given data.
Create a file at the given path relative to our repository, filled with the
given data.
:return: An absolute path to the created file.
"""
Expand Down
4 changes: 2 additions & 2 deletions test/performance/test_streams.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class TestObjDBPerformance(TestBigRepoR):

@with_rw_repo("HEAD", bare=True)
def test_large_data_streaming(self, rwrepo):
# TODO: This part overlaps with the same file in gitdb.test.performance.test_stream.
# It should be shared if possible.
# TODO: This part overlaps with the same file in
# gitdb.test.performance.test_stream. It should be shared if possible.
ldb = LooseObjectDB(osp.join(rwrepo.git_dir, "objects"))

for randomize in range(2):
Expand Down
4 changes: 2 additions & 2 deletions test/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ def test_add_unicode(self, rw_repo):
# https://github.com/gitpython-developers/GitPython/issues/147#issuecomment-68881897
# Therefore, it must be added using the Python implementation.
rw_repo.index.add([file_path])
# However, when the test winds down, rmtree fails to delete this file, which is recognized
# as ??? only.
# However, when the test winds down, rmtree fails to delete this file, which
# is recognized as ??? only.
else:
# On POSIX, we can just add Unicode files without problems.
rw_repo.git.add(rw_repo.working_dir)
Expand Down
4 changes: 2 additions & 2 deletions test/test_clone.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ def test_checkout_in_non_empty_dir(self, rw_dir):
garbage_file = non_empty_dir / "not-empty"
garbage_file.write_text("Garbage!")

# Verify that cloning into the non-empty dir fails while complaining about
# the target directory not being empty/non-existent.
# Verify that cloning into the non-empty dir fails while complaining about the
# target directory not being empty/non-existent.
try:
self.rorepo.clone(non_empty_dir)
except git.GitCommandError as exc:
Expand Down
16 changes: 9 additions & 7 deletions test/test_commit.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@

class TestCommitSerialization(TestBase):
def assert_commit_serialization(self, rwrepo, commit_id, print_performance_info=False):
"""Traverse all commits in the history of commit identified by commit_id and check
if the serialization works.
"""Traverse all commits in the history of commit identified by commit_id and
check if the serialization works.
:param print_performance_info: If True, we will show how fast we are.
"""
Expand Down Expand Up @@ -317,8 +317,9 @@ def test_count(self):
self.assertEqual(self.rorepo.tag("refs/tags/0.1.5").commit.count(), 143)

def test_list(self):
# This doesn't work anymore, as we will either attempt getattr with bytes, or compare 20 byte string
# with actual 20 byte bytes. This usage makes no sense anyway.
# This doesn't work anymore, as we will either attempt getattr with bytes, or
# compare 20 byte string with actual 20 byte bytes. This usage makes no sense
# anyway.
assert isinstance(
Commit.list_items(self.rorepo, "0.1.5", max_count=5)["5117c9c8a4d3af19a9958677e45cda9269de1541"],
Commit,
Expand Down Expand Up @@ -383,8 +384,8 @@ def test_serialization_unicode_support(self):

self.assertEqual(cmt.author.name, ncmt.author.name)
self.assertEqual(cmt.message, ncmt.message)
# Actually, it can't be printed in a shell as repr wants to have ascii only
# it appears.
# Actually, it can't be printed in a shell as repr wants to have ascii only it
# appears.
cmt.author.__repr__()

def test_invalid_commit(self):
Expand Down Expand Up @@ -498,7 +499,8 @@ def test_trailers(self):
KEY_2 = "Key"
VALUE_2 = "Value with inner spaces"

# Check that the following trailer example is extracted from multiple msg variations.
# Check that the following trailer example is extracted from multiple msg
# variations.
TRAILER = f"{KEY_1}: {VALUE_1_1}\n{KEY_2}: {VALUE_2}\n{KEY_1}: {VALUE_1_2}"
msgs = [
f"Subject\n\n{TRAILER}\n",
Expand Down
12 changes: 7 additions & 5 deletions test/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def _to_memcache(self, file_path):
return sio

def test_read_write(self):
# writer must create the exact same file as the one read before
# The writer must create the exact same file as the one read before.
for filename in ("git_config", "git_config_global"):
file_obj = self._to_memcache(fixture_path(filename))
with GitConfigParser(file_obj, read_only=False) as w_config:
Expand All @@ -56,7 +56,8 @@ def test_read_write(self):
self._to_memcache(fixture_path(filename)).getvalue(),
)

# Creating an additional config writer must fail due to exclusive access.
# Creating an additional config writer must fail due to exclusive
# access.
with self.assertRaises(IOError):
GitConfigParser(file_obj, read_only=False)

Expand Down Expand Up @@ -91,8 +92,8 @@ def test_includes_order(self):
r_config.read() # Enforce reading.
# Simple inclusions, again checking them taking precedence.
assert r_config.get_value("sec", "var0") == "value0_included"
# This one should take the git_config_global value since included
# values must be considered as soon as they get them.
# This one should take the git_config_global value since included values
# must be considered as soon as they get them.
assert r_config.get_value("diff", "tool") == "meld"
try:
# FIXME: Split this assertion out somehow and mark it xfail (or fix it).
Expand All @@ -109,7 +110,8 @@ def test_lock_reentry(self, rw_dir):
# Entering again locks the file again...
with gcp as cw:
cw.set_value("include", "some_other_value", "b")
# ...so creating an additional config writer must fail due to exclusive access.
# ...so creating an additional config writer must fail due to exclusive
# access.
with self.assertRaises(IOError):
GitConfigParser(fpl, read_only=False)
# but work when the lock is removed
Expand Down
23 changes: 12 additions & 11 deletions test/test_diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,18 +271,18 @@ def test_diff_unsafe_paths(self):
self.assertEqual(res[10].b_rawpath, b"path/\x80-invalid-unicode-path.txt")

# The "Moves"
# NOTE: The path prefixes a/ and b/ here are legit! We're actually
# verifying that it's not "a/a/" that shows up, see the fixture data.
self.assertEqual(res[11].a_path, "a/with spaces") # NOTE: path a/ here legit!
self.assertEqual(res[11].b_path, "b/with some spaces") # NOTE: path b/ here legit!
# NOTE: The path prefixes "a/" and "b/" here are legit! We're actually verifying
# that it's not "a/a/" that shows up; see the fixture data.
self.assertEqual(res[11].a_path, "a/with spaces") # NOTE: path "a/"" legit!
self.assertEqual(res[11].b_path, "b/with some spaces") # NOTE: path "b/"" legit!
self.assertEqual(res[12].a_path, "a/ending in a space ")
self.assertEqual(res[12].b_path, "b/ending with space ")
self.assertEqual(res[13].a_path, 'a/"with-quotes"')
self.assertEqual(res[13].b_path, 'b/"with even more quotes"')

def test_diff_patch_format(self):
# Test all of the 'old' format diffs for completeness - it should at least
# be able to deal with it.
# Test all of the 'old' format diffs for completeness - it should at least be
# able to deal with it.
fixtures = (
"diff_2",
"diff_2f",
Expand Down Expand Up @@ -345,8 +345,9 @@ def test_diff_submodule(self):
repo.create_tag("2")

diff = repo.commit("1").diff(repo.commit("2"))[0]
# If diff is unable to find the commit hashes (looks in wrong repo) the *_blob.size
# property will be a string containing exception text, an int indicates success.
# If diff is unable to find the commit hashes (looks in wrong repo) the
# *_blob.size property will be a string containing exception text, an int
# indicates success.
self.assertIsInstance(diff.a_blob.size, int)
self.assertIsInstance(diff.b_blob.size, int)

Expand Down Expand Up @@ -392,9 +393,9 @@ def test_diff_interface(self):
# END for each other side
# END for each commit

# Assert that we could always find at least one instance of the members we
# can iterate in the diff index - if not this indicates its not working correctly
# or our test does not span the whole range of possibilities.
# Assert that we could always find at least one instance of the members we can
# iterate in the diff index - if not this indicates its not working correctly or
# our test does not span the whole range of possibilities.
for key, value in assertion_map.items():
self.assertIsNotNone(value, "Did not find diff for %s" % key)
# END for each iteration type
Expand Down
26 changes: 14 additions & 12 deletions test/test_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ class Tutorials(TestBase):
def tearDown(self):
gc.collect()

# ACTUALLY skipped by git.util.rmtree (in local onerror function), from the last call to it via
# git.objects.submodule.base.Submodule.remove (at "handle separate bare repository"), line 1062.
# ACTUALLY skipped by git.util.rmtree (in local onerror function), from the last
# call to it via git.objects.submodule.base.Submodule.remove
# (at "handle separate bare repository"), line 1062.
#
# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
# "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
Expand All @@ -31,8 +32,8 @@ def test_init_repo_object(self, rw_dir):
from git import Repo

# rorepo is a Repo instance pointing to the git-python repository.
# For all you know, the first argument to Repo is a path to the repository
# you want to work with.
# For all you know, the first argument to Repo is a path to the repository you
# want to work with.
repo = Repo(self.rorepo.working_tree_dir)
assert not repo.bare
# ![1-test_init_repo_object]
Expand Down Expand Up @@ -149,8 +150,8 @@ def update(self, op_code, cur_count, max_count=None, message=""):
assert origin.exists()
for fetch_info in origin.fetch(progress=MyProgressPrinter()):
print("Updated %s to %s" % (fetch_info.ref, fetch_info.commit))
# Create a local branch at the latest fetched master. We specify the name statically, but you have all
# information to do it programmatically as well.
# Create a local branch at the latest fetched master. We specify the name
# statically, but you have all information to do it programmatically as well.
bare_master = bare_repo.create_head("master", origin.refs.master)
bare_repo.head.set_reference(bare_master)
assert not bare_repo.delete_remote(origin).exists()
Expand Down Expand Up @@ -188,9 +189,9 @@ def update(self, op_code, cur_count, max_count=None, message=""):
# submodules

# [14-test_init_repo_object]
# Create a new submodule and check it out on the spot, setup to track master branch of `bare_repo`.
# As our GitPython repository has submodules already that point to GitHub, make sure we don't
# interact with them.
# Create a new submodule and check it out on the spot, setup to track master
# branch of `bare_repo`. As our GitPython repository has submodules already that
# point to GitHub, make sure we don't interact with them.
for sm in cloned_repo.submodules:
assert not sm.remove().exists() # after removal, the sm doesn't exist anymore
sm = cloned_repo.create_submodule("mysubrepo", "path/to/subrepo", url=bare_repo.git_dir, branch="master")
Expand Down Expand Up @@ -424,8 +425,8 @@ def test_references_and_objects(self, rw_dir):
with origin.config_writer as cw:
cw.set("pushurl", "other_url")

# Please note that in Python 2, writing origin.config_writer.set(...) is totally safe.
# In py3 __del__ calls can be delayed, thus not writing changes in time.
# Please note that in Python 2, writing origin.config_writer.set(...) is totally
# safe. In py3 __del__ calls can be delayed, thus not writing changes in time.
# ![26-test_references_and_objects]

# [27-test_references_and_objects]
Expand Down Expand Up @@ -462,7 +463,8 @@ def test_references_and_objects(self, rw_dir):
# ![29-test_references_and_objects]

# [30-test_references_and_objects]
# Check out the branch using git-checkout. It will fail as the working tree appears dirty.
# Check out the branch using git-checkout.
# It will fail as the working tree appears dirty.
self.assertRaises(git.GitCommandError, repo.heads.master.checkout)
repo.heads.past_branch.checkout()
# ![30-test_references_and_objects]
Expand Down
3 changes: 2 additions & 1 deletion test/test_fun.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ def _assert_index_entries(self, entries, trees):
# END assert entry matches fully

def test_aggressive_tree_merge(self):
# Head tree with additions, removals and modification compared to its predecessor.
# Head tree with additions, removals and modification compared to its
# predecessor.
odb = self.rorepo.odb
HC = self.rorepo.commit("6c1faef799095f3990e9970bc2cb10aa0221cf9c")
H = HC.tree
Expand Down
22 changes: 14 additions & 8 deletions test/test_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,21 +253,27 @@ def test_it_avoids_upcasing_unrelated_environment_variable_names(self):
if old_name == old_name.upper():
raise RuntimeError("test bug or strange locale: old_name invariant under upcasing")

# Step 1: Set the environment variable in this parent process. Because os.putenv is a thin
# wrapper around a system API, os.environ never sees the variable in this parent
# process, so the name is not upcased even on Windows.
# Step 1
#
# Set the environment variable in this parent process. Because os.putenv is a
# thin wrapper around a system API, os.environ never sees the variable in this
# parent process, so the name is not upcased even on Windows.
os.putenv(old_name, "1")

# Step 2: Create the child process that inherits the environment variable. The child uses
# GitPython, and we are testing that it passes the variable with the exact original
# name to its own child process (the grandchild).
# Step 2
#
# Create the child process that inherits the environment variable. The child
# uses GitPython, and we are testing that it passes the variable with the exact
# original name to its own child process (the grandchild).
cmdline = [
sys.executable,
fixture_path("env_case.py"), # Contains steps 3 and 4.
self.rorepo.working_dir,
old_name,
]
pair_text = subprocess.check_output(cmdline, shell=False, text=True) # Run steps 3 and 4.

# Run steps 3 and 4.
pair_text = subprocess.check_output(cmdline, shell=False, text=True)

new_name = pair_text.split("=")[0]
self.assertEqual(new_name, old_name)
Expand Down Expand Up @@ -668,7 +674,7 @@ def test_successful_default_refresh_invalidates_cached_version_info(self):
# as unintended shell expansions can occur, and is deprecated. Instead,
# use a custom command, by setting the GIT_PYTHON_GIT_EXECUTABLE
# environment variable to git.cmd or by passing git.cmd's full path to
# git.refresh. Or wrap the script with a .exe shim.
# git.refresh. Or wrap the script with a .exe shim.)
stack.enter_context(mock.patch.object(Git, "USE_SHELL", True))

new_git = Git()
Expand Down
Loading

0 comments on commit 5cf5b60

Please sign in to comment.