From d24a7e6c5a415ece9ad377d13b20daefc944fdd4 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 29 Sep 2018 19:55:04 -0300 Subject: [PATCH 1/4] Issue warning if Monkeypatch.setenv/delenv receive non-strings in Python 2 Fixes the bug described in: https://github.com/tox-dev/tox/pull/1025#discussion_r221273830 Which is more evident when using `unicode_literals`. --- changelog/4056.bugfix.rst | 4 ++++ src/_pytest/monkeypatch.py | 14 ++++++++++++++ testing/test_monkeypatch.py | 26 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 changelog/4056.bugfix.rst diff --git a/changelog/4056.bugfix.rst b/changelog/4056.bugfix.rst new file mode 100644 index 000000000..f018a4776 --- /dev/null +++ b/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``. diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index 67279003f..60feb0802 100644 --- a/src/_pytest/monkeypatch.py +++ b/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,6 +212,15 @@ 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 @@ -216,6 +228,7 @@ class MonkeyPatch(object): 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 +238,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): diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index adf10e4d0..f0f63023d 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -3,6 +3,8 @@ import os import sys import textwrap +import six + import pytest from _pytest.monkeypatch import MonkeyPatch @@ -192,6 +194,30 @@ def test_delenv(): del os.environ[name] +@pytest.mark.skipif(six.PY3, reason="Python 2 only test") +class TestEnvironKeysWarning(object): + """ + os.environ needs keys to be native strings, otherwise it will cause problems with other modules (notably + subprocess). We only test this behavior on Python 2, because Python 3 raises an error right away. + """ + + VAR_NAME = u"PYTEST_INTERNAL_MY_VAR" + + 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") + + 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_prepend(): import os From bc009a8582b4787c409528bf9c0d1bc6792d35ce Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 30 Sep 2018 13:06:48 -0300 Subject: [PATCH 2/4] Fix test to comply with pypy 6.0 --- testing/code/test_source.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/code/test_source.py b/testing/code/test_source.py index 14f06acd0..66e880e05 100644 --- a/testing/code/test_source.py +++ b/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" From 9d971d33bec34715715bf5c5c269bc73fca00c36 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 1 Oct 2018 18:45:08 -0300 Subject: [PATCH 3/4] Hide internal pytest.warns traceback --- src/_pytest/recwarn.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/_pytest/recwarn.py b/src/_pytest/recwarn.py index 0eee4c841..7a0586697 100644 --- a/src/_pytest/recwarn.py +++ b/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: From 1a323fbd3c52db051ae32e2b27275717b891b322 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 1 Oct 2018 18:48:41 -0300 Subject: [PATCH 4/4] Show a warning when non-str is given to Monkeypatch.setenv --- src/_pytest/monkeypatch.py | 10 +++++++++- testing/acceptance_test.py | 4 ++-- testing/test_cacheprovider.py | 14 +++++++------- testing/test_junitxml.py | 2 +- testing/test_monkeypatch.py | 27 ++++++++++++++++++++------- 5 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index 60feb0802..4bd6b607d 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -225,7 +225,15 @@ class MonkeyPatch(object): """ 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) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 8a9585be2..332af27b5 100644 --- a/testing/acceptance_test.py +++ b/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*"]) diff --git a/testing/test_cacheprovider.py b/testing/test_cacheprovider.py index 5d73dc846..2444d8bc1 100644 --- a/testing/test_cacheprovider.py +++ b/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("FAILTEST", fail_run) + monkeypatch.setenv("FAILIMPORT", str(fail_import)) + 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("FAILTEST", fail_run) + monkeypatch.setenv("FAILIMPORT", str(fail_import)) + monkeypatch.setenv("FAILTEST", str(fail_run)) result = testdir.runpytest("-q", "--lf", *args) config = testdir.parseconfigure() diff --git a/testing/test_junitxml.py b/testing/test_junitxml.py index 3928548a8..2304c4a50 100644 --- a/testing/test_junitxml.py +++ b/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) diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index f0f63023d..853c0192f 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -165,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" @@ -194,15 +195,16 @@ def test_delenv(): del os.environ[name] -@pytest.mark.skipif(six.PY3, reason="Python 2 only test") -class TestEnvironKeysWarning(object): +class TestEnvironWarnings(object): """ - os.environ needs keys to be native strings, otherwise it will cause problems with other modules (notably - subprocess). We only test this behavior on Python 2, because Python 3 raises an error right away. + 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, @@ -210,6 +212,7 @@ class TestEnvironKeysWarning(object): ): 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, @@ -217,14 +220,24 @@ class TestEnvironKeysWarning(object): ): 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