Repository: tox-dev/tox-travis

Issue:
Rename toxenv
We use the name `toxenv` in several places, because the `TOXENV` environment variable is the mechanism I used to implement the primary feature of Env Detection. When #78 is merged, it will no longer be used, except to make sure we don't do anything if it's already been given. So let's not use it as a name.

Some places that I know it's used:

* [ ] The name of the module that implements env detection. (Rename to `detect` or `envlist`)
* [ ] The name of the function that implements env detection. (Name the same as the module above)
* [ ] The name of the file with the documentation for env detection. (Name the same as those above)
* [ ] The name of the setting for the `[travis:after]` section that says which envs to look for. (Either the same as the above, or perhaps just `env`. Keep the current name for backward compatibility.)

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=h1) Report
> Merging [#83](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=desc) into [master](https://codecov.io/gh/tox-dev/tox-travis/commit/ffe844135832585a8bad0b5bd56edecbb47ba654?src=pr&el=desc) will **decrease** coverage by `0.42%`.
> The diff coverage is `63.63%`.

[![Impacted file tree graph](https://codecov.io/gh/tox-dev/tox-travis/pull/83/graphs/tree.svg?src=pr&width=650&token=CBMEkiKkWy&height=150)](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   77.27%   76.84%   -0.43%     
==========================================
  Files           5        5              
  Lines         198      203       +5     
  Branches       46       48       +2     
==========================================
+ Hits          153      156       +3     
- Misses         38       40       +2     
  Partials        7        7
```


| [Impacted Files](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/tox\_travis/after.py](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=tree#diff-c3JjL3RveF90cmF2aXMvYWZ0ZXIucHk=) | `56.47% <0%> (-1.37%)` | :arrow_down: |
| [src/tox\_travis/detect.py](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=tree#diff-c3JjL3RveF90cmF2aXMvZGV0ZWN0LnB5) | `95.5% <100%> (ø)` | |
| [src/tox\_travis/hooks.py](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=tree#diff-c3JjL3RveF90cmF2aXMvaG9va3MucHk=) | `86.66% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=footer). Last update [ffe8441...b94d6fc](https://codecov.io/gh/tox-dev/tox-travis/pull/83?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

rpkilby: Two thoughts:
-  A part of my says the output should be tested for the warnings, but it seems straightforward enough to not warrant tests.
- Would it make sense to name the files `envlist` instead of `detect`? eg, 
  ```python
  from envlist import detect_envlist
  ```
ryanhiebert: Yeah, I've got to figure out how to test this section. Right now the after feature is considered "experimental", so I don't have it fully tested (especially the hooks and monkeypatch).

`envlist` was the original plan, as I specified on #80. My main driver for doing something different was #84 , considering how I'd like to spell that flag. I like `--auto-envlist` less than `--detect`, and `--envlist` isn't a clear option IMO. Avoiding multiple words was something that I liked with `--detect`. Perhaps the flag needs to be `--detect-envlist` (or the negative). In that case it would be multiple words, but I'd have no issue renaming the files to `envlist` then.


ryanhiebert: As I noted on #84, `envlist` is a good name for the feature, so I've updated this PR to use `envlist` as the name that we're renaming toxenv to.
ryanhiebert: I'm glad that I added tests for the changes in `travis:after`. There were some bugs that needed to be addressed.

Patch:
diff --git a/docs/after.rst b/docs/after.rst
index 664b166..4de27af 100644
--- a/docs/after.rst
+++ b/docs/after.rst
@@ -101,7 +101,7 @@ that you are shipping a working release.
 The accepted configuration keys
 in the ``[travis:after]`` section are:
 
-* ``toxenv``. Match with the running toxenvs.
+* ``envlist``. Match with the running toxenvs.
   Expansion is allowed, and if set *all* environments listed
   must be run in the current Tox run.
 * ``travis``. Match with known Travis factors,
diff --git a/docs/toxenv.rst b/docs/envlist.rst
similarity index 100%
rename from docs/toxenv.rst
rename to docs/envlist.rst
diff --git a/docs/index.rst b/docs/index.rst
index ebf5347..90714fc 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -6,7 +6,7 @@ Topics
 .. toctree::
    :maxdepth: 1
 
-   toxenv
+   envlist
    after
    contributing
    history
diff --git a/src/tox_travis/after.py b/src/tox_travis/after.py
index caf3750..53def96 100644
--- a/src/tox_travis/after.py
+++ b/src/tox_travis/after.py
@@ -22,16 +22,16 @@ INCOMPLETE_TRAVIS_ENVIRONMENT = 34
 JOBS_FAILED = 35
 
 
-def travis_after_monkeypatch():
+def travis_after_monkeypatch(ini, envlist):
     """Monkeypatch the Tox session to wait for jobs to finish."""
     import tox.session
     real_subcommand_test = tox.session.Session.subcommand_test
 
     def subcommand_test(self):
         retcode = real_subcommand_test(self)
-        if retcode == 0 and self.config.option.travis_after:
+        if retcode == 0:
             # No need to run if the tests failed anyway
-            travis_after(self.config.envlist, self.config._cfg)
+            travis_after(envlist, ini)
         return retcode
     tox.session.Session.subcommand_test = subcommand_test
 
@@ -83,8 +83,13 @@ def after_config_matches(envlist, ini):
     if not section:
         return False  # Never wait if it's not configured
 
-    if 'toxenv' in section:
-        required = set(split_env(section['toxenv']))
+    if 'envlist' in section or 'toxenv' in section:
+        if 'toxenv' in section:
+            print('The "toxenv" key of the [travis:after] section is '
+                  'deprecated in favor of the "envlist" key.', file=sys.stderr)
+
+        toxenv = section.get('toxenv')
+        required = set(split_env(section.get('envlist', toxenv) or ''))
         actual = set(envlist)
         if required - actual:
             return False
@@ -120,8 +125,8 @@ def get_job_statuses(github_token, api_url, build_id,
         build = get_json('{api_url}/builds/{build_id}'.format(
             api_url=api_url, build_id=build_id), auth=auth)
         jobs = [job for job in build['jobs']
-                if job['number'] != job_number
-                and not job['allow_failure']]  # Ignore allowed failures
+                if job['number'] != job_number and
+                not job['allow_failure']]  # Ignore allowed failures
         if all(job['finished_at'] for job in jobs):
             break  # All the jobs have completed
         elif any(job['state'] != 'passed'
diff --git a/src/tox_travis/toxenv.py b/src/tox_travis/envlist.py
similarity index 90%
rename from src/tox_travis/toxenv.py
rename to src/tox_travis/envlist.py
index 8bed3c7..2e1ea1e 100644
--- a/src/tox_travis/toxenv.py
+++ b/src/tox_travis/envlist.py
@@ -1,4 +1,4 @@
-"""Default TOXENV based on the Travis environment."""
+"""Default Tox envlist based on the Travis environment."""
 from __future__ import print_function
 from itertools import product
 import os
@@ -9,13 +9,8 @@ from tox.config import _split_env as split_env
 from .utils import TRAVIS_FACTORS, parse_dict
 
 
-def default_toxenv(config):
+def detect_envlist(ini):
     """Default envlist automatically based on the Travis environment."""
-    if 'TOXENV' in os.environ or config.option.env:
-        return  # Skip any processing if already set
-
-    ini = config._cfg
-
     # Find the envs that tox knows about
     declared_envs = get_declared_envs(ini)
 
@@ -26,13 +21,8 @@ def default_toxenv(config):
     desired_envs = ['-'.join(env) for env in product(*desired_factors)]
 
     # Find matching envs
-    matched = match_envs(declared_envs, desired_envs,
-                         passthru=len(desired_factors) == 1)
-
-    # Make the envconfig for undeclared matched envs
-    autogen_envconfigs(config, set(matched) - set(config.envconfigs))
-
-    config.envlist = matched
+    return match_envs(declared_envs, desired_envs,
+                      passthru=len(desired_factors) == 1)
 
 
 def autogen_envconfigs(config, envs):
@@ -232,9 +222,7 @@ def env_matches(declared, desired):
     return all(factor in declared_factors for factor in desired_factors)
 
 
-def override_ignore_outcome(config):
-    """Override ignore_outcome if unignore_outcomes is set to True."""
-    travis_reader = tox.config.SectionReader("travis", config._cfg)
-    if travis_reader.getbool('unignore_outcomes', False):
-        for envconfig in config.envconfigs.values():
-            envconfig.ignore_outcome = False
+def override_ignore_outcome(ini):
+    """Decide whether to override ignore_outcomes."""
+    travis_reader = tox.config.SectionReader("travis", ini)
+    return travis_reader.getbool('unignore_outcomes', False)
diff --git a/src/tox_travis/hooks.py b/src/tox_travis/hooks.py
index c1f00d5..3d83075 100644
--- a/src/tox_travis/hooks.py
+++ b/src/tox_travis/hooks.py
@@ -1,8 +1,11 @@
 """Tox hook implementations."""
+from __future__ import print_function
 import os
+import sys
 import tox
-from .toxenv import (
-    default_toxenv,
+from .envlist import (
+    detect_envlist,
+    autogen_envconfigs,
     override_ignore_outcome,
 )
 from .hacks import pypy_version_monkeypatch
@@ -11,7 +14,7 @@ from .after import travis_after_monkeypatch
 
 @tox.hookimpl
 def tox_addoption(parser):
-    """Add arguments and override TOXENV."""
+    """Add arguments and needed monkeypatches."""
     parser.add_argument(
         '--travis-after', dest='travis_after', action='store_true',
         help='Exit successfully after all Travis jobs complete successfully.')
@@ -23,8 +26,27 @@ def tox_addoption(parser):
 @tox.hookimpl
 def tox_configure(config):
     """Check for the presence of the added options."""
-    if 'TRAVIS' in os.environ:
-        if config.option.travis_after:
-            travis_after_monkeypatch()
-        default_toxenv(config)
-        override_ignore_outcome(config)
+    if 'TRAVIS' not in os.environ:
+        return
+
+    ini = config._cfg
+
+    # envlist
+    if 'TOXENV' not in os.environ and not config.option.env:
+        envlist = detect_envlist(ini)
+        undeclared = set(envlist) - set(config.envconfigs)
+        if undeclared:
+            print('Matching undeclared envs is deprecated. Be sure all the '
+                  'envs that Tox should run are declared in the tox config.',
+                  file=sys.stderr)
+            autogen_envconfigs(config, undeclared)
+        config.envlist = envlist
+
+    # Override ignore_outcomes
+    if override_ignore_outcome(ini):
+        for envconfig in config.envconfigs.values():
+            envconfig.ignore_outcome = False
+
+    # after
+    if config.option.travis_after:
+        travis_after_monkeypatch(ini, config.envlist)


Test patch:
diff --git a/tests/test_after.py b/tests/test_after.py
index 06fb01c..50e6738 100644
--- a/tests/test_after.py
+++ b/tests/test_after.py
@@ -1,6 +1,10 @@
 """Tests of the --travis-after flag."""
 import pytest
-from tox_travis.after import travis_after
+import py
+from tox_travis.after import (
+    travis_after,
+    after_config_matches,
+)
 
 
 class TestAfter:
@@ -294,3 +298,106 @@ class TestAfter:
         assert excinfo.value.code == 35
         out, err = capsys.readouterr()
         assert 'Some jobs were not successful.' in out
+
+    def test_after_config_matches_unconfigured(self):
+        """Skip quickly when after section is unconfigured."""
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert not after_config_matches(['py36'], ini)
+
+    def test_after_config_matches_toxenv_match(self, capsys):
+        """Test that it works using the legacy toxenv setting.
+
+        It should also give a warning message.
+        """
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'toxenv = py36\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert after_config_matches(['py36'], ini)
+        out, err = capsys.readouterr()
+        msg = 'The "toxenv" key of the [travis:after] section is deprecated'
+        assert msg in err
+
+    def test_after_config_matches_toxenv_nomatch(self, capsys):
+        """Test that it doesn't work using the legacy toxenv setting.
+
+        It should also give a warning message.
+        """
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'toxenv = py36\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert not after_config_matches(['py35'], ini)
+        out, err = capsys.readouterr()
+        msg = 'The "toxenv" key of the [travis:after] section is deprecated'
+        assert msg in err
+
+    def test_after_config_matches_envlist_match(self):
+        """Test that it works."""
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'envlist = py36\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert after_config_matches(['py36'], ini)
+
+    def test_after_config_matches_envlist_nomatch(self):
+        """Test that it doesn't work."""
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'envlist = py36\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert not after_config_matches(['py35'], ini)
+
+    def test_after_config_matches_env_match(self, monkeypatch):
+        """Test that it works."""
+        monkeypatch.setenv('TRAVIS_PYTHON_VERSION', '3.6')
+        monkeypatch.setenv('DJANGO', '1.11')
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'travis =\n'
+            '    python: 3.6\n'
+            'env =\n'
+            '    DJANGO: 1.11\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert after_config_matches(['py36'], ini)
+
+    def test_after_config_matches_env_nomatch(self, monkeypatch):
+        """Test that it doesn't work."""
+        monkeypatch.setenv('TRAVIS_PYTHON_VERSION', '3.5')
+        monkeypatch.setenv('DJANGO', '1.11')
+        inistr = (
+            '[tox]\n'
+            'envlist = py36\n'
+            '\n'
+            '[travis:after]\n'
+            'travis =\n'
+            '    python: 3.6\n'
+            'env =\n'
+            '    DJANGO: 1.11\n'
+        )
+        ini = py.iniconfig.IniConfig('', data=inistr)
+        assert not after_config_matches(['py35'], ini)