Fix substitution problems introduced in 2.8 (#599)

* #595 fix by updating implementation of #515

The way #515 was implemented did not play nicely with env var substitution. This might render the former additions completely or partially obsolete but I did not check this yet. First let's fix that bug.

* fix flakes

(cherry picked from commit e902f04)

* #595 fix by updating implementation of #515

The way #515 was implemented did not play nicely with env var substitution. This might render the former additions completely or partially obsolete but I did not check this yet. First let's fix that bug.

Instead of crashing early on testenv creation if the substitution the missing substitution keys are added to a list attached to TestenvConfig. This way it is not necessary anymore to provide everything needed to resolve all testenvs but instead only what is needed to resolve all testenvs that are part of this restrun.

Implementation notes:

The first implementation used a module level mapping from envname to missing substitutions, which was ugly. Instead the information is bubbled up as an exception where it can be handled the way it is needed in the contexts where the information can be either translated into a crashing error or attached to the object which should carry the information (in this case TestenvConfig).

Atm a missing substitution in a testenv is not being left empty but filled with a special value 'TOX_MISSING_SUBSTITUTION', this is not exactly necessary, but I don't feel comfortable enough yet to leave it empty, this can help with debugging potential problems that might arise from this change.

* #595 missed a bit

Crash only if actually trying to run a testenv with missing substitutions

* fix error on execution

Instead of crashing the whole thing again - only later, do it the right way by setting venv.status with the error to be reported later.

* #595 tests and last touches

* catch MissingSubstition from all method calls for consistency
* set status and stop execution in setupvenv if substutions are missing to prevent errors occuring there with missing substitutions

Author: obestwalter

tests/test_config.py

@@ -444,18 +444,20 @@ def test_getdict(self, tmpdir, newconfig):
         x = reader.getdict("key3", {1: 2})
         assert x == {1: 2}
 
-    def test_getstring_environment_substitution(self, monkeypatch, newconfig):
-        monkeypatch.setenv("KEY1", "hello")
-        config = newconfig("""
-            [section]
-            key1={env:KEY1}
-            key2={env:KEY2}
-        """)
-        reader = SectionReader("section", config._cfg)
-        x = reader.getstring("key1")
-        assert x == "hello"
+    def test_normal_env_sub_works(self, monkeypatch, newconfig):
+        monkeypatch.setenv("VAR", "hello")
+        config = newconfig("[section]\nkey={env:VAR}")
+        assert SectionReader("section", config._cfg).getstring("key") == "hello"
+
+    def test_missing_env_sub_raises_config_error_in_non_testenv(self, newconfig):
+        config = newconfig("[section]\nkey={env:VAR}")
         with pytest.raises(tox.exception.ConfigError):
-            reader.getstring("key2")
+            SectionReader("section", config._cfg).getstring("key")
+
+    def test_missing_env_sub_populates_missing_subs(self, newconfig):
+        config = newconfig("[testenv:foo]\ncommands={env:VAR}")
+        print(SectionReader("section", config._cfg).getstring("commands"))
+        assert config.envconfigs['foo'].missing_subs == ['VAR']
 
     def test_getstring_environment_substitution_with_default(self, monkeypatch, newconfig):
         monkeypatch.setenv("KEY1", "hello")

tests/test_z_cmdline.py

@@ -871,3 +871,10 @@ def test_envtmpdir(initproj, cmd):
 
     result = cmd.run("tox")
     assert not result.ret
+
+
+def test_missing_env_fails(initproj, cmd):
+    initproj("foo", filedefs={'tox.ini': "[testenv:foo]\ncommands={env:VAR}"})
+    result = cmd.run("tox")
+    assert result.ret == 1
+    result.stdout.fnmatch_lines(["*foo: unresolvable substitution(s): 'VAR'*"])

tox/__init__.py

@@ -14,12 +14,18 @@ class Error(Exception):
         def __str__(self):
             return "%s: %s" % (self.__class__.__name__, self.args[0])
 
+    class MissingSubstitution(Exception):
+        FLAG = 'TOX_MISSING_SUBSTITUTION'
+        """placeholder for debugging configurations"""
+        def __init__(self, name):
+            self.name = name
+
     class ConfigError(Error):
         """ error in tox configuration. """
     class UnsupportedInterpreter(Error):
-        "signals an unsupported Interpreter"
+        """signals an unsupported Interpreter"""
     class InterpreterNotFound(Error):
-        "signals that an interpreter could not be found"
+        """signals that an interpreter could not be found"""
     class InvocationError(Error):
         """ an error while invoking a script. """
     class MissingFile(Error):
@@ -35,4 +41,5 @@ def __init__(self, message):
             self.message = message
             super(exception.MinVersionError, self).__init__(message)
 
+
 from tox.session import main as cmdline  # noqa

tox/config.py

@@ -609,6 +609,13 @@ def __init__(self, envname, config, factors, reader):
         #: set of factors
         self.factors = factors
         self._reader = reader
+        self.missing_subs = []
+        """Holds substitutions that could not be resolved.
+
+        Pre 2.8.1 missing substitutions crashed with a ConfigError although this would not be a
+        problem if the env is not part of the current testrun. So we need to remember this and
+        check later when the testenv is actually run and crash only then.
+        """
 
     def get_envbindir(self):
         """ path to directory where scripts/binaries reside. """
@@ -791,9 +798,7 @@ def __init__(self, config, inipath):
             section = testenvprefix + name
             factors = set(name.split('-'))
             if section in self._cfg or factors <= known_factors:
-                config.envconfigs[name] = \
-                    self.make_envconfig(name, section, reader._subs, config,
-                                        replace=name in config.envlist)
+                config.envconfigs[name] = self.make_envconfig(name, section, reader._subs, config)
 
         all_develop = all(name in config.envconfigs
                           and config.envconfigs[name].usedevelop
@@ -813,33 +818,31 @@ def make_envconfig(self, name, section, subs, config, replace=True):
         factors = set(name.split('-'))
         reader = SectionReader(section, self._cfg, fallbacksections=["testenv"],
                                factors=factors)
-        vc = TestenvConfig(config=config, envname=name, factors=factors, reader=reader)
-        reader.addsubstitutions(**subs)
-        reader.addsubstitutions(envname=name)
-        reader.addsubstitutions(envbindir=vc.get_envbindir,
-                                envsitepackagesdir=vc.get_envsitepackagesdir,
-                                envpython=vc.get_envpython)
-
+        tc = TestenvConfig(name, config, factors, reader)
+        reader.addsubstitutions(
+            envname=name, envbindir=tc.get_envbindir, envsitepackagesdir=tc.get_envsitepackagesdir,
+            envpython=tc.get_envpython, **subs)
         for env_attr in config._testenv_attr:
             atype = env_attr.type
-            if atype in ("bool", "path", "string", "dict", "dict_setenv", "argv", "argvlist"):
-                meth = getattr(reader, "get" + atype)
-                res = meth(env_attr.name, env_attr.default, replace=replace)
-            elif atype == "space-separated-list":
-                res = reader.getlist(env_attr.name, sep=" ")
-            elif atype == "line-list":
-                res = reader.getlist(env_attr.name, sep="\n")
-            else:
-                raise ValueError("unknown type %r" % (atype,))
-
-            if env_attr.postprocess:
-                res = env_attr.postprocess(testenv_config=vc, value=res)
-            setattr(vc, env_attr.name, res)
-
+            try:
+                if atype in ("bool", "path", "string", "dict", "dict_setenv", "argv", "argvlist"):
+                    meth = getattr(reader, "get" + atype)
+                    res = meth(env_attr.name, env_attr.default, replace=replace)
+                elif atype == "space-separated-list":
+                    res = reader.getlist(env_attr.name, sep=" ")
+                elif atype == "line-list":
+                    res = reader.getlist(env_attr.name, sep="\n")
+                else:
+                    raise ValueError("unknown type %r" % (atype,))
+                if env_attr.postprocess:
+                    res = env_attr.postprocess(testenv_config=tc, value=res)
+            except tox.exception.MissingSubstitution as e:
+                tc.missing_subs.append(e.name)
+                res = e.FLAG
+            setattr(tc, env_attr.name, res)
             if atype in ("path", "string"):
                 reader.addsubstitutions(**{env_attr.name: res})
-
-        return vc
+        return tc
 
     def _getenvdata(self, reader):
         envstr = self.config.option.env                                \
@@ -961,8 +964,7 @@ def getdict(self, name, default=None, sep="\n", replace=True):
 
     def getdict_setenv(self, name, default=None, sep="\n", replace=True):
         value = self.getstring(name, None, replace=replace, crossonly=True)
-        definitions = self._getdict(value, default=default, sep=sep,
-                                    replace=replace)
+        definitions = self._getdict(value, default=default, sep=sep, replace=replace)
         self._setenv = SetenvDict(definitions, reader=self)
         return self._setenv
 
@@ -1042,9 +1044,15 @@ def _replace(self, value, name=None, section_name=None, crossonly=False):
         section_name = section_name if section_name else self.section_name
         self._subststack.append((section_name, name))
         try:
-            return Replacer(self, crossonly=crossonly).do_replace(value)
-        finally:
+            replaced = Replacer(self, crossonly=crossonly).do_replace(value)
             assert self._subststack.pop() == (section_name, name)
+        except tox.exception.MissingSubstitution:
+            if not section_name.startswith(testenvprefix):
+                raise tox.exception.ConfigError(
+                    "substitution env:%r: unknown or recursive definition in "
+                    "section %r." % (value, section_name))
+            raise
+        return replaced
 
 
 class Replacer:
@@ -1112,20 +1120,14 @@ def _replace_match(self, match):
     def _replace_env(self, match):
         envkey = match.group('substitution_value')
         if not envkey:
-            raise tox.exception.ConfigError(
-                'env: requires an environment variable name')
-
+            raise tox.exception.ConfigError('env: requires an environment variable name')
         default = match.group('default_value')
-
         envvalue = self.reader.get_environ_value(envkey)
-        if envvalue is None:
-            if default is None:
-                raise tox.exception.ConfigError(
-                    "substitution env:%r: unknown environment variable %r "
-                    " or recursive definition." %
-                    (envkey, envkey))
+        if envvalue is not None:
+            return envvalue
+        if default is not None:
             return default
-        return envvalue
+        raise tox.exception.MissingSubstitution(envkey)
 
     def _substitute_from_other_section(self, key):
         if key.startswith("[") and "]" in key:

tox/session.py

@@ -440,6 +440,12 @@ def make_emptydir(self, path):
             path.ensure(dir=1)
 
     def setupenv(self, venv):
+        if venv.envconfig.missing_subs:
+            venv.status = (
+                "unresolvable substitution(s): %s. "
+                "Environment variables are missing or defined recursively." %
+                (','.join(["'%s'" % m for m in venv.envconfig.missing_subs])))
+            return
         if not venv.matching_platform():
             venv.status = "platform mismatch"
             return  # we simply omit non-matching platforms