René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"test_git_work_tree_env renders os.environ inert in unpatching attempt","articleBody":"`TestRepo.test_git_work_tree_env` contains this code, which is intended to patch and unpatch two environment variables:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/test/test_repo.py#L1341-L1350\r\n\r\nHowever, that does not unpatch the variables, and more importantly, it prevents writes through `os.environ` from ever actually setting environment variables (to be inherited automatically by subprocesses) for the rest of the process's lifetime.\r\n\r\n```text\r\n\u003e\u003e\u003e import os\r\n\u003e\u003e\u003e type(os.environ)\r\n\u003cclass 'os._Environ'\u003e\r\n\u003e\u003e\u003e oldenv = os.environ.copy()\r\n\u003e\u003e\u003e type(oldenv)\r\n\u003cclass 'dict'\u003e\r\n\u003e\u003e\u003e os.system(\"printenv FOO\")\r\n256\r\n\u003e\u003e\u003e os.environ[\"FOO\"] = \"bar\"\r\n\u003e\u003e\u003e os.system(\"printenv FOO\")\r\nbar\r\n0\r\n\u003e\u003e\u003e os.environ = oldenv\r\n\u003e\u003e\u003e os.system(\"printenv FOO\")\r\nbar\r\n0\r\n\u003e\u003e\u003e os.environ[\"BAZ\"] = \"quux\"\r\n\u003e\u003e\u003e os.system(\"printenv BAZ\")\r\n256\r\n```\r\n\r\nThis happens because, all types in Python being reference types, the `environ` attribute of `os` merely *refers to* the object of a special mutable mapping type that updates the process's environment variables when mutated. Calling its `copy` method constructs a `dict` object with the same items. Assigning that `dict` back to `os.environ` does not modify the original `os.environ` object in any way, but instead causes the `environ` attribute of `os` to point to the `dict` object from that point forward. From then on, writes to `os.environ` have no effect on the running process's environment variables.\r\n\r\nYet, this does not cause any other tests to fail, nor do any currently passing tests fail as a result of fixing it. I believe this is for three reasons, which I list in descending order of what I *guess* to be their significance:\r\n\r\n1. When GitPython runs `git`, it doesn't rely on automatically passing its own environment variables to the `git` subprocess. Instead, it builds a modified environment based on its own and passes that explicitly. The way it finds out about its own environment is by consulting `os.environ`, which doesn't actually have to work for setting environment variables, because `Popen` is doing it:\r\n\r\n   https://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/git/cmd.py#L948-L955\r\n\r\n   https://github.com/gitpython-developers/GitPython/blob/a5a646494393478c65f26cd3a921f3505219d3e1/git/cmd.py#L987-L989\r\n\r\n2. When GitPython, or any library it uses if that library is written in Python, or the testing framework, accesses and changes environment variables for use within the process, that is also nearly always via `os.environ`, so again, it's okay if the real environment is not used.\r\n\r\n3. The test is in `test_submodules.py`, and test modules are usually exercised in alphabetical order, with only `test_tree.py` and `test_util.py` coming after it.\r\n\r\nThis only affects the tests, and it can be solved in any way appropriate for tests. Specifically, it can be solved--and also that test code significantly simplified--by patching `os.environ` with `unittest.mock.patch.dict`. We cannot use this in the code of the `git` module, because it upcases environment variable names (#1646). But it is not a problem to use it in tests (besides the test in #1650 that #1646 is fixed), and it is already being used in a few places in the test suite.\r\n\r\n---\r\n\r\nThe way I found out about this was that `flake8` reported it once I removed `test/` from its excluded directories:\r\n\r\n```text\r\ntest/test_repo.py:1350:13: B003 Assigning to `os.environ` doesn't clear the environment. Subprocesses are going to see outdated variables, in disagreement with the current process. Use `os.environ.clear()` or the `env=` argument to Popen.\r\n            os.environ = oldenv\r\n            ^\r\n```\r\n\r\nThe specific suggestions aren't quite applicable here (we do need to mutate `os.environ`, and to do so less drastically than with `clear`), but that message nonetheless accurately identifies the problem, which seems previously to have gone undetected. I think it is a good idea to enable `flake8` for `test/` as well as `git/` (though perhaps sometime soon we might manage to replace `flake8` with [`ruff`](https://docs.astral.sh/ruff/)). I plan to include a fix for this, and also for less serious, mostly merely stylistic issues found while examining `test/` with the help of `flake8`, in the same PR that fixes #1670.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-09-21T09:46:18.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":0},"url":"https://github.com/1671/GitPython/issues/1671"}