pytester: remove special handling of env during inner runs

[blueyed]

No description provided.

[RonnyPfannschmidt]

sorry i missed that detail before

[RonnyPfannschmidt]

sorry, also pressed the wrong button

[blueyed]

I've extended test_testdir_respects_monkeypatch to show what this is meant to achieve: if a test uses monkeypatch.setenv("HOME", …) itself, it should also be used by the inner test run then.
Reworked the patch for this, not using the global monkeypatch fixture, but only for env updates.

[nicoddemus]

I've extended test_testdir_respects_monkeypatch to show what this is meant to achieve: if a test uses monkeypatch.setenv("HOME", …) itself, it should also be used by the inner test run then.
Reworked the patch for this, not using the global monkeypatch fixture, but only for env updates.

Hmmm what if we just set HOME and USERPROFILE during Testdir.__init__? Then tests which call monkeypatch.setenv("HOME", ...) will override the initial setting... something like this:

diff --git a/src/_pytest/pytester.py b/src/_pytest/pytester.py
index ca780a9f5..63384261b 100644
--- a/src/_pytest/pytester.py
+++ b/src/_pytest/pytester.py
@@ -359,8 +359,8 @@ def LineMatcher_fixture(request: FixtureRequest) -> "Type[LineMatcher]":
 
 
 @pytest.fixture
-def testdir(request: FixtureRequest, tmpdir_factory) -> "Testdir":
-    return Testdir(request, tmpdir_factory)
+def testdir(request: FixtureRequest, tmpdir_factory, monkeypatch) -> "Testdir":
+    return Testdir(request, tmpdir_factory, monkeypatch)
 
 
 @pytest.fixture
@@ -524,7 +524,7 @@ class Testdir:
     class TimeoutExpired(Exception):
         pass
 
-    def __init__(self, request, tmpdir_factory):
+    def __init__(self, request, tmpdir_factory, monkeypatch):
         self.request = request
         self._mod_collections = WeakKeyDictionary()
         name = request.function.__name__
@@ -547,7 +547,12 @@ class Testdir:
 
         # Environment (updates) for inner runs.
         tmphome = str(self.tmpdir)
-        self._env_run_update = {"HOME": tmphome, "USERPROFILE": tmphome}
+        self.monkeypatch = monkeypatch
+        if sys.platform.startswith("win"):
+            self.monkeypatch.setenv("USERPROFILE", tmphome)
+        else:
+            self.monkeypatch.setenv("HOME", tmphome)
+        self.monkeypatch.delenv("PYTEST_ADDOPTS", raising=False)
 
     def __repr__(self):
         return "<Testdir {!r}>".format(self.tmpdir)
@@ -853,12 +858,6 @@ class Testdir:
         plugins = list(plugins)
         finalizers = []
         try:
-            # Do not load user config (during runs only).
-            mp_run = MonkeyPatch()
-            for k, v in self._env_run_update.items():
-                mp_run.setenv(k, v)
-            finalizers.append(mp_run.undo)
-
             # Any sys.module or sys.path changes done while running pytest
             # inline should be reverted after the test run completes to avoid
             # clashing with later inline tests run within the same pytest test,
@@ -1091,7 +1090,6 @@ class Testdir:
         env["PYTHONPATH"] = os.pathsep.join(
             filter(None, [os.getcwd(), env.get("PYTHONPATH", "")])
         )
-        env.update(self._env_run_update)
         kw["env"] = env
 
         if stdin is Testdir.CLOSE_STDIN:
@@ -1261,11 +1259,7 @@ class Testdir:
             pytest.skip("pexpect.spawn not available")
         logfile = self.tmpdir.join("spawn.out").open("wb")
 
-        # Do not load user config.
-        env = os.environ.copy()
-        env.update(self._env_run_update)
-
-        child = pexpect.spawn(cmd, logfile=logfile, env=env)
+        child = pexpect.spawn(cmd, logfile=logfile)
         self.request.addfinalizer(logfile.close)
         child.timeout = expect_timeout
         return child
diff --git a/testing/test_pytester.py b/testing/test_pytester.py
index 758e999dc..8a044cb1a 100644
--- a/testing/test_pytester.py
+++ b/testing/test_pytester.py
@@ -674,3 +674,26 @@ def test_run_result_repr():
         repr(r) == "<RunResult ret=99 len(stdout.lines)=3"
         " len(stderr.lines)=4 duration=0.50s>"
     )
+
+
+@pytest.mark.parametrize("method", ("setenv", "delenv"))
+def test_testdir_respects_monkeypatch(method, testdir, monkeypatch):
+    testdir.makepyfile("""
+        import os
+        def test_home():
+            print("HOME:", os.environ.get("HOME"))
+    """)
+
+    if method == "setenv":
+        monkeypatch.setenv("HOME", "anotherhome")
+    else:
+        assert method == "delenv"
+        monkeypatch.delenv("HOME", raising=False)
+
+    result = testdir.runpytest("-vs")
+
+    if method == "setenv":
+        result.stdout.fnmatch_lines("*HOME: anotherhome*")
+    else:
+        assert method == "delenv"
+        result.stdout.fnmatch_lines("*HOME: None*")

This of course changes the behavior a bit, because the environment variables will be changed for the entire run, not only for the inner run...

[nicoddemus]

Regardless if we follow my previous suggestion about using monkeypatch during Testdir.__init__, can we have a more user facing test here? The test I posted also works on your branch, and tests the actual behavior we want:

@pytest.mark.parametrize("method", ("setenv", "delenv"))
def test_testdir_respects_monkeypatch(method, testdir, monkeypatch):
    testdir.makepyfile("""
        import os
        def test_home():
            print("HOME:", os.environ.get("HOME"))
    """)

    if method == "setenv":
        monkeypatch.setenv("HOME", "anotherhome")
    else:
        assert method == "delenv"
        monkeypatch.delenv("HOME", raising=False)

    result = testdir.runpytest("-vs")

    if method == "setenv":
        result.stdout.fnmatch_lines("*HOME: anotherhome*")
    else:
        assert method == "delenv"
        result.stdout.fnmatch_lines("*HOME: None*")

[blueyed]

Not strictly opposed - my initial test was meant to be a unit test for _get_env_run_update (which would be gone then though). It's much faster though (1.2s for 100 runs (200 passes), compared to 20s for the above).

[nicoddemus]

Well I fear that by testing the inner mechanism and not testing the expected behavior, we might introduce a regression later and fail to notice. But I will leave this one to you.

[blueyed]

I think this is covered by existing/changed tests.

[blueyed]

Hmmm what if we just set HOME and USERPROFILE during Testdir.__init__? Then tests which call monkeypatch.setenv("HOME", ...) will override the initial setting... something like this:

…

This of course changes the behavior a bit, because the environment variables will be changed for the entire run, not only for the inner run...

Which makes a difference for ~/.pdbrc etc. IIRC this was the main motivation for doing this during inner runs / for subprocesses only.
But it's still an issue in there then, of course, so e.g. with pdbpp I am using / experimenting with using the "real home" for its config (via pwd.getpwuid(os.getuid()).pw_dir). Therefore I'm not opposed to keeping it separate really (from the main monkeypatch used there).

It does not need to use the existing/global monkeypatch fixture in the first place anymore then though, does it?

[blueyed]

I'll change it, removing _env_run_update.

[blueyed]

@RonnyPfannschmidt @nicoddemus
Is this good now?

[blueyed]

@nicoddemus
Thanks.
Had to resolve a conflict - please check again.

[nicoddemus]

LGTM 👍

[blueyed]

Thanks for the reviews - this unblocks #6164 then.