Repository: tox-dev/tox

Issue:
2.8.0: {[testenv]setenv} substitution broken 
While installing tempest as part of devstack install, below commnd blows up with an error - 
`tox -r --notest -efull`

Error -
```
2017-09-01 18:22:20.661 | ++ lib/tempest:install_tempest:612          :   pushd /opt/stack/new/tempest
2017-09-01 18:22:20.661 | ~/tempest ~/devstack
2017-09-01 18:22:20.662 | ++ lib/tempest:install_tempest:613          :   tox -r --notest -efull
2017-09-01 18:22:21.720 | Traceback (most recent call last):
2017-09-01 18:22:21.720 |   File "/usr/local/bin/tox", line 11, in <module>
2017-09-01 18:22:21.720 |     sys.exit(cmdline())
2017-09-01 18:22:21.720 |   File "/usr/local/lib/python2.7/dist-packages/tox/session.py", line 38, in main
2017-09-01 18:22:21.720 |     config = prepare(args)
2017-09-01 18:22:21.720 |   File "/usr/local/lib/python2.7/dist-packages/tox/session.py", line 26, in prepare
2017-09-01 18:22:21.720 |     config = parseconfig(args)
2017-09-01 18:22:21.720 |   File "/usr/local/lib/python2.7/dist-packages/tox/config.py", line 242, in parseconfig
2017-09-01 18:22:21.720 |     parseini(config, inipath)
2017-09-01 18:22:21.720 |   File "/usr/local/lib/python2.7/dist-packages/tox/config.py", line 796, in __init__
2017-09-01 18:22:21.721 |     replace=name in config.envlist)
2017-09-01 18:22:21.721 |   File "/usr/local/lib/python2.7/dist-packages/tox/config.py", line 827, in make_envconfig
2017-09-01 18:22:21.721 |     res = meth(env_attr.name, env_attr.default, replace=replace)
2017-09-01 18:22:21.721 |   File "/usr/local/lib/python2.7/dist-packages/tox/config.py", line 964, in getdict_setenv
2017-09-01 18:22:21.721 |     definitions = self._getdict(value, default=default, sep=sep)
2017-09-01 18:22:21.721 |   File "/usr/local/lib/python2.7/dist-packages/tox/config.py", line 975, in _getdict
2017-09-01 18:22:21.721 |     name, rest = line.split('=', 1)
2017-09-01 18:22:21.721 | ValueError: need more than 1 value to unpack
2017-09-01 18:22:21.733 | + lib/tempest:install_tempest:1            :   exit_trap
```


Logs link - http://logs.openstack.netapp.com/ci-logs/logs/65/499765/5/upstream-check/cinder-cDOT-FCP/3566c47/logs/devstacklog.txt.gz




Discussion:

Patch:
diff --git a/CHANGELOG b/CHANGELOG
index 6afb70b4..3b95f4c3 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,7 +1,7 @@
 Not released yet
 ----------------
 
-- #p598: fix problems with implementation of #515.
+- #p599: fix problems with implementation of #515.
          Substitutions from other sections were not made anymore if they were not in `envlist`.
          Thanks to Clark Boylan (@cboylan) for helping to get this fixed (#p597).
 
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


Test patch:
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'*"])