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

5 files changed, 78 insertions, 54 deletions

diff --git a/tests/test_config.py b/tests/test_config.py
index 6fa64571..2533c962 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -444,18 +444,20 @@ class TestIniParser:
         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")
diff --git a/tests/test_z_cmdline.py b/tests/test_z_cmdline.py
index 49e7641f..97e0712b 100644
--- a/tests/test_z_cmdline.py
+++ b/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'*"])
diff --git a/tox/__init__.py b/tox/__init__.py
index 73d095eb..98f1246a 100644
--- a/tox/__init__.py
+++ b/tox/__init__.py
@@ -14,12 +14,18 @@ class 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 @@ class exception:
             self.message = message
             super(exception.MinVersionError, self).__init__(message)
 
+
 from tox.session import main as cmdline  # noqa
diff --git a/tox/config.py b/tox/config.py
index 9f0024c0..d5a6baf8 100755
--- a/tox/config.py
+++ b/tox/config.py
@@ -609,6 +609,13 @@ class TestenvConfig:
         #: 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 @@ class parseini:
             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 @@ class parseini:
         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 @@ class SectionReader:
 
     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 @@ class SectionReader:
         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 @@ class Replacer:
     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:
diff --git a/tox/session.py b/tox/session.py
index 269a7705..60baaeee 100644
--- a/tox/session.py
+++ b/tox/session.py
@@ -440,6 +440,12 @@ class Session:
             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