From 953d1616e7385ec6aac1e7a6212c689874c9409c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 19:04:15 -0400 Subject: [PATCH 1/7] Comment what `TestInstallation._set_up_venv` does So that the meaning of the `venv.sources` accesses in `test_installation` is more readily clear. --- test/test_installation.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test_installation.py b/test/test_installation.py index ae6472e98..8231b3512 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -64,10 +64,14 @@ def test_installation(self, rw_dir): @staticmethod def _set_up_venv(rw_dir): + # Initialize the virtual environment. venv = VirtualEnvironment(rw_dir, with_pip=True) + + # Make its src directory a symlink to our own top-level source tree. os.symlink( os.path.dirname(os.path.dirname(__file__)), venv.sources, target_is_directory=True, ) + return venv From 84632c78512e070a7aaa9ff1dd046c77293a58a4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 19:06:58 -0400 Subject: [PATCH 2/7] Extract `subprocess.run` logic repeated in `test_installation` This creates a function (technically, a callable `partial` object) for `test_installation` to use instead of repeating `subproces.run` keyword arguments all the time. This relates directly to steps in `_set_up_venv`, and it's makes about as much sense to do it there as in `test_installation`, so it is placed (and described) in `_set_up_venv`. --- test/test_installation.py | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/test/test_installation.py b/test/test_installation.py index 8231b3512..b428f413a 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -2,6 +2,7 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ import ast +import functools import os import subprocess @@ -11,35 +12,23 @@ class TestInstallation(TestBase): @with_rw_directory def test_installation(self, rw_dir): - venv = self._set_up_venv(rw_dir) + venv, run = self._set_up_venv(rw_dir) - result = subprocess.run( - [venv.pip, "install", "."], - stdout=subprocess.PIPE, - cwd=venv.sources, - ) + result = run([venv.pip, "install", "."]) self.assertEqual( 0, result.returncode, msg=result.stderr or result.stdout or "Can't install project", ) - result = subprocess.run( - [venv.python, "-c", "import git"], - stdout=subprocess.PIPE, - cwd=venv.sources, - ) + result = run([venv.python, "-c", "import git"]) self.assertEqual( 0, result.returncode, msg=result.stderr or result.stdout or "Self-test failed", ) - result = subprocess.run( - [venv.python, "-c", "import gitdb; import smmap"], - stdout=subprocess.PIPE, - cwd=venv.sources, - ) + result = run([venv.python, "-c", "import gitdb; import smmap"]) self.assertEqual( 0, result.returncode, @@ -49,11 +38,7 @@ def test_installation(self, rw_dir): # Even IF gitdb or any other dependency is supplied during development by # inserting its location into PYTHONPATH or otherwise patched into sys.path, # make sure it is not wrongly inserted as the *first* entry. - result = subprocess.run( - [venv.python, "-c", "import sys; import git; print(sys.path)"], - stdout=subprocess.PIPE, - cwd=venv.sources, - ) + result = run([venv.python, "-c", "import sys; import git; print(sys.path)"]) syspath = result.stdout.decode("utf-8").splitlines()[0] syspath = ast.literal_eval(syspath) self.assertEqual( @@ -74,4 +59,11 @@ def _set_up_venv(rw_dir): target_is_directory=True, ) - return venv + # Create a convenience function to run commands in it. + run = functools.partial( + subprocess.run, + stdout=subprocess.PIPE, + cwd=venv.sources, + ) + + return venv, run From a2ba4804a6544642b400dced6eac0f4f1ad28750 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 19:11:16 -0400 Subject: [PATCH 3/7] Have `test_installation` test that operations produce no warnings By setting the `PYTHONWARNINGS` environment variable to `error` in each of the subprocess invocations. This is strictly stronger than passing `-Werror` for the `python` commands, because it automatically applies to subprocesses (unless they are created with a sanitized environment or otherwise with one in which `PYTHONWARNINGS` has been customized), and because it works for `pip` automatically. Importantly, this will cause warnings internal to Python subprocesses created by `pip` to be treated as errors. It should thus surface any warnings coming from the `setuptools` backend. --- test/test_installation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_installation.py b/test/test_installation.py index b428f413a..5ba8a82b0 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -64,6 +64,7 @@ def _set_up_venv(rw_dir): subprocess.run, stdout=subprocess.PIPE, cwd=venv.sources, + env={**os.environ, "PYTHONWARNINGS": "error"}, ) return venv, run From a0e08fe90135149de203a3adfb21a5a254cb1bdb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 19:48:39 -0400 Subject: [PATCH 4/7] Show more information when `test_installation` fails Previously, it attempted to show stderr unless empty, first falling back to stdout unless empty, then falling back to the prewritten summary identifying the specific assertion. This now has the `test_installation` assertions capture stderr as well as stdout, handle standard streams as text rather than binary, and show more information when failing, always distinguishing where the information came from: the summary, then labeled captured stdout (empty or not), then labeled captured stderr (empty or not). That applies to all but the last assertion, which does not try to show information differently when it fails, but is simplified to do the right thing now that `subprocess.run` is using text streams. (This subtly changes its semantics, but overall it should be as effective as before at finding the `sys.path` woe it anticipates.) --- test/test_installation.py | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/test/test_installation.py b/test/test_installation.py index 5ba8a82b0..39797c134 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -15,31 +15,19 @@ def test_installation(self, rw_dir): venv, run = self._set_up_venv(rw_dir) result = run([venv.pip, "install", "."]) - self.assertEqual( - 0, - result.returncode, - msg=result.stderr or result.stdout or "Can't install project", - ) + self._check_result(result, "Can't install project") result = run([venv.python, "-c", "import git"]) - self.assertEqual( - 0, - result.returncode, - msg=result.stderr or result.stdout or "Self-test failed", - ) + self._check_result(result, "Self-test failed") result = run([venv.python, "-c", "import gitdb; import smmap"]) - self.assertEqual( - 0, - result.returncode, - msg=result.stderr or result.stdout or "Dependencies not installed", - ) + self._check_result(result, "Dependencies not installed") # Even IF gitdb or any other dependency is supplied during development by # inserting its location into PYTHONPATH or otherwise patched into sys.path, # make sure it is not wrongly inserted as the *first* entry. result = run([venv.python, "-c", "import sys; import git; print(sys.path)"]) - syspath = result.stdout.decode("utf-8").splitlines()[0] + syspath = result.stdout.splitlines()[0] syspath = ast.literal_eval(syspath) self.assertEqual( "", @@ -63,8 +51,17 @@ def _set_up_venv(rw_dir): run = functools.partial( subprocess.run, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True, cwd=venv.sources, env={**os.environ, "PYTHONWARNINGS": "error"}, ) return venv, run + + def _check_result(self, result, failure_summary): + self.assertEqual( + 0, + result.returncode, + f"{failure_summary}\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}", + ) From 6826b594e2ef43b0972f29f335e7be51c9574303 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 20:05:56 -0400 Subject: [PATCH 5/7] Improve failure message whitespace clarity --- test/test_installation.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/test_installation.py b/test/test_installation.py index 39797c134..da0b86ed2 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -63,5 +63,11 @@ def _check_result(self, result, failure_summary): self.assertEqual( 0, result.returncode, - f"{failure_summary}\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}", + self._prepare_failure_message(result, failure_summary), ) + + @staticmethod + def _prepare_failure_message(result, failure_summary): + stdout = result.stdout.rstrip() + stderr = result.stderr.rstrip() + return f"{failure_summary}\n\nstdout:\n{stdout}\n\nstderr:\n{stderr}" From d0868bd5d6a43c3d95a3eaddab1389a0ef76d8f0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 20:25:55 -0400 Subject: [PATCH 6/7] Remove deprecated license classifier GitPython project metadata specify the project's license primarily through the value of `license`, currently passed as an argument to `setuptools.setup`, which holds the string `"BSD-3-Clause"`. This is an SPDX license identifier readily understood by both humans and machines. The PyPI trove classifier "License :: OSI Approved :: BSD License" has also been specified in `setup.py`. However, this is not ideal, because: 1. It does not identify a specific license. There are multiple "BSD" licenses in use, with BSD-2-Clause and BSD-3-Clause both in very wide use. 2. It is no longer recommended to use a trove classifier to indicate a license. The use of license classifiers (even unambiguous ones) has been deprecated. See: - https://packaging.python.org/en/latest/specifications/core-metadata/#classifier-multiple-use - https://peps.python.org/pep-0639/#deprecate-license-classifiers This commit removes the classifier. The license itself is of course unchanged, as is the `license` value of `"BSD-3-Clause"`. (An expected effect of this change is that, starting in the next release of GitPython, PyPI may show "License: BSD-3-Clause" instead of the current text "License: BSD License (BSD-3-Clause)".) This change fixes a warning issued by a subprocess of `pip` when installing the package. The warning, until this change, could be observed by running `pip install . -v` or `pip install -e . -v` and examining the verbose output, or by running `pip install .` or `pip install -e .` with the `PYTHONWARNINGS` environment variable set to `error`: SetuptoolsDeprecationWarning: License classifiers are deprecated. !! ******************************************************************************** Please consider removing the following classifiers in favor of a SPDX license expression: License :: OSI Approved :: BSD License See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details. ******************************************************************************** !! (In preceding commits, `test_installation` has been augmented to set that environment variable, surfacing the error. This change should allow that test to pass, unless it finds other problems.) --- setup.py | 1 - 1 file changed, 1 deletion(-) diff --git a/setup.py b/setup.py index f28fedb85..a7b1eab00 100755 --- a/setup.py +++ b/setup.py @@ -95,7 +95,6 @@ def _stamp_version(filename: str) -> None: # "Development Status :: 7 - Inactive", "Environment :: Console", "Intended Audience :: Developers", - "License :: OSI Approved :: BSD License", "Operating System :: OS Independent", "Operating System :: POSIX", "Operating System :: Microsoft :: Windows", From b21c32a302dcd91d52636e5f91e89ae129654bd2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Jun 2025 23:37:27 -0400 Subject: [PATCH 7/7] Pass assertion `msg` as a keyword argument in `test_installation` It was originally made explicit like this, but it ended up becoming position in my last few commits. This restores that clearer aspect of how it was written before, while keeping all the other changes. --- test/test_installation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_installation.py b/test/test_installation.py index da0b86ed2..7c82bd403 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -63,7 +63,7 @@ def _check_result(self, result, failure_summary): self.assertEqual( 0, result.returncode, - self._prepare_failure_message(result, failure_summary), + msg=self._prepare_failure_message(result, failure_summary), ) @staticmethod