Merge pull request #4056 from nicoddemus/unicode-vars

Ensure Monkeypatch setenv and delenv use bytes keys in Python 2

changelog/4056.bugfix.rst

@@ -0,0 +1,4 @@
+``MonkeyPatch.setenv`` and ``MonkeyPatch.delenv`` issue a warning if the environment variable name is not ``str`` on Python 2.
+
+In Python 2, adding ``unicode`` keys to ``os.environ`` causes problems with ``subprocess`` (and possible other modules),
+making this a subtle bug specially susceptible when used with ``from __future__ import unicode_literals``.

src/_pytest/monkeypatch.py

@@ -4,9 +4,12 @@ from __future__ import absolute_import, division, print_function
 import os
 import sys
 import re
+import warnings
 from contextlib import contextmanager
 
 import six
+
+import pytest
 from _pytest.fixtures import fixture
 
 RE_IMPORT_ERROR_NAME = re.compile("^No module named (.*)$")
@@ -209,13 +212,31 @@ class MonkeyPatch(object):
             self._setitem.append((dic, name, dic.get(name, notset)))
             del dic[name]
 
+    def _warn_if_env_name_is_not_str(self, name):
+        """On Python 2, warn if the given environment variable name is not a native str (#4056)"""
+        if six.PY2 and not isinstance(name, str):
+            warnings.warn(
+                pytest.PytestWarning(
+                    "Environment variable name {!r} should be str".format(name)
+                )
+            )
+
     def setenv(self, name, value, prepend=None):
         """ Set environment variable ``name`` to ``value``.  If ``prepend``
         is a character, read the current environment variable value
         and prepend the ``value`` adjoined with the ``prepend`` character."""
-        value = str(value)
+        if not isinstance(value, str):
+            warnings.warn(
+                pytest.PytestWarning(
+                    "Environment variable value {!r} should be str, converted to str implicitly".format(
+                        value
+                    )
+                )
+            )
+            value = str(value)
         if prepend and name in os.environ:
             value = value + prepend + os.environ[name]
+        self._warn_if_env_name_is_not_str(name)
         self.setitem(os.environ, name, value)
 
     def delenv(self, name, raising=True):
@@ -225,6 +246,7 @@ class MonkeyPatch(object):
         If ``raising`` is set to False, no exception will be raised if the
         environment variable is missing.
         """
+        self._warn_if_env_name_is_not_str(name)
         self.delitem(os.environ, name, raising=raising)
 
     def syspath_prepend(self, path):

src/_pytest/recwarn.py

@@ -212,6 +212,8 @@ class WarningsChecker(WarningsRecorder):
     def __exit__(self, *exc_info):
         super(WarningsChecker, self).__exit__(*exc_info)
 
+        __tracebackhide__ = True
+
         # only check if we're not currently handling an exception
         if all(a is None for a in exc_info):
             if self.expected_warning is not None:

testing/acceptance_test.py

@@ -577,7 +577,7 @@ class TestInvocationVariants(object):
             return what
 
         empty_package = testdir.mkpydir("empty_package")
-        monkeypatch.setenv("PYTHONPATH", join_pythonpath(empty_package))
+        monkeypatch.setenv("PYTHONPATH", str(join_pythonpath(empty_package)))
         # the path which is not a package raises a warning on pypy;
         # no idea why only pypy and not normal python warn about it here
         with warnings.catch_warnings():
@@ -586,7 +586,7 @@ class TestInvocationVariants(object):
         assert result.ret == 0
         result.stdout.fnmatch_lines(["*2 passed*"])
 
-        monkeypatch.setenv("PYTHONPATH", join_pythonpath(testdir))
+        monkeypatch.setenv("PYTHONPATH", str(join_pythonpath(testdir)))
         result = testdir.runpytest("--pyargs", "tpkg.test_missing", syspathinsert=True)
         assert result.ret != 0
         result.stderr.fnmatch_lines(["*not*found*test_missing*"])

testing/code/test_source.py

@@ -129,7 +129,7 @@ def test_source_strip_multiline():
 def test_syntaxerror_rerepresentation():
     ex = pytest.raises(SyntaxError, _pytest._code.compile, "xyz xyz")
     assert ex.value.lineno == 1
-    assert ex.value.offset in (4, 7)  # XXX pypy/jython versus cpython?
+    assert ex.value.offset in (4, 5, 7)  # XXX pypy/jython versus cpython?
     assert ex.value.text.strip(), "x x"
 
 

testing/test_cacheprovider.py

@@ -215,7 +215,7 @@ def test_cache_show(testdir):
 
 class TestLastFailed(object):
     def test_lastfailed_usecase(self, testdir, monkeypatch):
-        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", 1)
+        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", "1")
         p = testdir.makepyfile(
             """
             def test_1():
@@ -301,7 +301,7 @@ class TestLastFailed(object):
         assert "test_a.py" not in result.stdout.str()
 
     def test_lastfailed_difference_invocations(self, testdir, monkeypatch):
-        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", 1)
+        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", "1")
         testdir.makepyfile(
             test_a="""\
             def test_a1():
@@ -335,7 +335,7 @@ class TestLastFailed(object):
         result.stdout.fnmatch_lines(["*1 failed*1 desel*"])
 
     def test_lastfailed_usecase_splice(self, testdir, monkeypatch):
-        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", 1)
+        monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", "1")
         testdir.makepyfile(
             """\
             def test_1():
@@ -474,8 +474,8 @@ class TestLastFailed(object):
         )
 
         def rlf(fail_import, fail_run):
-            monkeypatch.setenv("FAILIMPORT", fail_import)
+            monkeypatch.setenv("FAILIMPORT", str(fail_import))
-            monkeypatch.setenv("FAILTEST", fail_run)
+            monkeypatch.setenv("FAILTEST", str(fail_run))
 
             testdir.runpytest("-q")
             config = testdir.parseconfigure()
@@ -519,8 +519,8 @@ class TestLastFailed(object):
         )
 
         def rlf(fail_import, fail_run, args=()):
-            monkeypatch.setenv("FAILIMPORT", fail_import)
+            monkeypatch.setenv("FAILIMPORT", str(fail_import))
-            monkeypatch.setenv("FAILTEST", fail_run)
+            monkeypatch.setenv("FAILTEST", str(fail_run))
 
             result = testdir.runpytest("-q", "--lf", *args)
             config = testdir.parseconfigure()

testing/test_junitxml.py

@@ -850,7 +850,7 @@ def test_logxml_path_expansion(tmpdir, monkeypatch):
     assert xml_tilde.logfile == home_tilde
 
     # this is here for when $HOME is not set correct
-    monkeypatch.setenv("HOME", tmpdir)
+    monkeypatch.setenv("HOME", str(tmpdir))
     home_var = os.path.normpath(os.path.expandvars("$HOME/test.xml"))
 
     xml_var = LogXML("$HOME%stest.xml" % tmpdir.sep, None)

testing/test_monkeypatch.py

@@ -3,6 +3,8 @@ import os
 import sys
 import textwrap
 
+import six
+
 import pytest
 from _pytest.monkeypatch import MonkeyPatch
 
@@ -163,7 +165,8 @@ def test_delitem():
 
 def test_setenv():
     monkeypatch = MonkeyPatch()
-    monkeypatch.setenv("XYZ123", 2)
+    with pytest.warns(pytest.PytestWarning):
+        monkeypatch.setenv("XYZ123", 2)
     import os
 
     assert os.environ["XYZ123"] == "2"
@@ -192,13 +195,49 @@ def test_delenv():
             del os.environ[name]
 
 
+class TestEnvironWarnings(object):
+    """
+    os.environ keys and values should be native strings, otherwise it will cause problems with other modules (notably
+    subprocess). On Python 2 os.environ accepts anything without complaining, while Python 3 does the right thing
+    and raises an error.
+    """
+
+    VAR_NAME = u"PYTEST_INTERNAL_MY_VAR"
+
+    @pytest.mark.skipif(six.PY3, reason="Python 2 only test")
+    def test_setenv_unicode_key(self, monkeypatch):
+        with pytest.warns(
+            pytest.PytestWarning,
+            match="Environment variable name {!r} should be str".format(self.VAR_NAME),
+        ):
+            monkeypatch.setenv(self.VAR_NAME, "2")
+
+    @pytest.mark.skipif(six.PY3, reason="Python 2 only test")
+    def test_delenv_unicode_key(self, monkeypatch):
+        with pytest.warns(
+            pytest.PytestWarning,
+            match="Environment variable name {!r} should be str".format(self.VAR_NAME),
+        ):
+            monkeypatch.delenv(self.VAR_NAME, raising=False)
+
+    def test_setenv_non_str_warning(self, monkeypatch):
+        value = 2
+        msg = (
+            "Environment variable value {!r} should be str, converted to str implicitly"
+        )
+        with pytest.warns(pytest.PytestWarning, match=msg.format(value)):
+            monkeypatch.setenv(str(self.VAR_NAME), value)
+
+
 def test_setenv_prepend():
     import os
 
     monkeypatch = MonkeyPatch()
-    monkeypatch.setenv("XYZ123", 2, prepend="-")
+    with pytest.warns(pytest.PytestWarning):
+        monkeypatch.setenv("XYZ123", 2, prepend="-")
     assert os.environ["XYZ123"] == "2"
-    monkeypatch.setenv("XYZ123", 3, prepend="-")
+    with pytest.warns(pytest.PytestWarning):
+        monkeypatch.setenv("XYZ123", 3, prepend="-")
     assert os.environ["XYZ123"] == "3-2"
     monkeypatch.undo()
     assert "XYZ123" not in os.environ