Move toxenv to tox_configure hook #78

ryanhiebert · 2017-10-17T05:15:02Z

The tox_configure hook is much better suited for the purpose of toxenv. This PR is some refactors to aid, and a move of the bulk of the project to being triggered via the tox_configure hook. This is based off of the idea of #61, so it also closes #71. Thanks @rpkilby!

The abandonment of TOXENV to communicate with Tox also means that this also fixes #44 as a side-effect.

ryanhiebert · 2017-10-17T14:14:32Z

There's still a few test failures that I need to determine the cause of. I should be able to take a closer look at the failures after work today.

ryanhiebert · 2017-10-21T13:04:10Z

Due to the change of setting the config.envlist directly, instead of going through the TOXENV environment variable, this addresses the core concern of #75 as well.

codecov-io · 2017-10-21T13:18:53Z

Codecov Report

Merging #78 into master will increase coverage by 1.95%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   75.26%   77.22%   +1.95%     
==========================================
  Files           5        5              
  Lines         186      202      +16     
  Branches       45       46       +1     
==========================================
+ Hits          140      156      +16     
  Misses         39       39              
  Partials        7        7

Impacted Files	Coverage Δ
src/tox_travis/hooks.py	`86.66% <100%> (ø)`	⬆️
src/tox_travis/toxenv.py	`95.45% <100%> (+1.01%)`	⬆️
src/tox_travis/after.py	`57.64% <40%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b936589...2e80f1f. Read the comment docs.

ryanhiebert · 2017-10-21T13:31:21Z

I finally got the bugs worked out, and this is ready for review!

ryanhiebert · 2017-10-21T14:42:18Z

I realized that I'd broken the After feature by not setting TOXENV, so now #44 is also fixed here as a side-effect of getting this done.

This is a cleaner method of configuring the desired envs to run, by overriding tox's config.envlist. There are a few things worth noting: 1. Because we override envlist directly, instead of proxying through the TOXENV environment variable, it eliminates an inconsistency where all envs run when no environments are detected to run, because TOXENV was set to the empty string and ignored. 2. Because we override envlist later, we must also check manually for envs passed directly into tox, so we don't override in that case (config.option.env). 3. To maintain backward compatibility, envs that don't exist in the tox.ini aren't automatically created like they are when using the TOXENV environment variable, so we have to manually add the envconfigs necessary for those. 4. All references to TOXENV are abandoned.

ryanhiebert · 2017-10-23T16:02:39Z

@rpkilby : Sorry if I'm bugging, I just want to make sure you know that I'm happy with where this is now, and I want to use it as the basis for future work since it's a pretty significant change fundamentally (though hopefully without unintended side-effects), and that it's ready for review when you have the time to do so.

If there's anyone else willing to review, that would be helpful too.

rpkilby

Everything looks good, just looking for clarification.

rpkilby · 2017-10-24T01:23:49Z

tests/test_toxenv.py

@@ -416,7 +416,7 @@ def test_travis_ignore_outcome(self, tmpdir, monkeypatch):
        """Test ignore_outcome without setting obey_outcomes."""
        tox_ini = tox_ini_ignore_outcome
        with self.configure(
-            tmpdir, monkeypatch, tox_ini, travis_language='generic'
+            tmpdir, monkeypatch, tox_ini, travis_version='3.4'


Out of curiosity, what was the reason behind this change?

Because the generic language wasn't even being used in the tox config, so it turns out that it was relying on the broken behavior from #75. And that broken behavior was fixed as a side-effect of this Pull Request, so this needed to be changed to match.

Thanks, that makes sense.

rpkilby · 2017-10-24T01:48:39Z

src/tox_travis/toxenv.py


+    # Make the envconfig for undeclared matched envs
+    autogen_envconfigs(config, set(matched) - set(config.envconfigs))


This part puzzles me, comparing this to #61, I don't understand what exactly autgen_envconfigs does and why it's necessary. It's not clear to me what's depending on the generated envconfigs, and why this wasn't necessary in #61.

This is a case that #61 didn't cover, and exists for backward compatibility with the first release of tox-travis. With that first release, you would declare what environments got run for each version of Python. It didn't look at the environments that were listed at all, it just sent the string in the configuration verbatim, through the TOXENV environment variable. If the specified environment is not declared in the tox config, Tox will still happily run that environment, doing the best it can based on the settings it has.

When we switched to matching based on factors, so that by default on Python 3.4 all envs with py34 in the name would run, that obviously was inherently in conflict with the original mechanism. However, I was able to maintain partial backward compatibility by saying that if there was only one "factor" being matched (python, which is automatic), and no environments matched it, then whatever the setting is will be used verbatim, in the same way as was done in the original version.

This is a wart, but fixing it means a breaking change. This workaround allows me to get this change in without having to worry about a breaking change at this time.

Oh, and as far as what it actually does. Before the tox_configure hook, Tox looks at all the environments, and creates a envconfig for each one. The envconfig is the configuration of how to run the environment, with default settings resolved. What this does is notice when we've ended up with an envconfig that Tox didn't know about to begin with, and make an envconfig for it the same way Tox would have if it had been specified in TOXENV.

This is a wart, but fixing it means a breaking change. This workaround allows me to get this change in without having to worry about a breaking change at this time.

Gotcha, this is compatibility code, and would presumably not be present in the next major release.

ryanhiebert · 2017-10-25T14:35:46Z

@rpkilby : If you're satisfied with my responses, can you update your review to an approval?

rpkilby

This all makes sense to me now, thanks for the clarification.

rpkilby · 2017-10-25T23:53:51Z

tests/test_toxenv.py

@@ -416,7 +416,7 @@ def test_travis_ignore_outcome(self, tmpdir, monkeypatch):
        """Test ignore_outcome without setting obey_outcomes."""
        tox_ini = tox_ini_ignore_outcome
        with self.configure(
-            tmpdir, monkeypatch, tox_ini, travis_language='generic'
+            tmpdir, monkeypatch, tox_ini, travis_version='3.4'


Thanks, that makes sense.

rpkilby · 2017-10-26T00:36:44Z

src/tox_travis/toxenv.py


+    # Make the envconfig for undeclared matched envs
+    autogen_envconfigs(config, set(matched) - set(config.envconfigs))


This is a wart, but fixing it means a breaking change. This workaround allows me to get this change in without having to worry about a breaking change at this time.

Gotcha, this is compatibility code, and would presumably not be present in the next major release.

ryanhiebert force-pushed the tox-configure branch 2 times, most recently from ad81d66 to c7f83b1 Compare October 17, 2017 13:50

ryanhiebert mentioned this pull request Oct 18, 2017

Refactor #79

Merged

ryanhiebert force-pushed the tox-configure branch from c7f83b1 to 08b8017 Compare October 21, 2017 01:41

Change config variable to ini

f28adbb

ryanhiebert force-pushed the tox-configure branch from 08b8017 to a9adbbc Compare October 21, 2017 12:59

ryanhiebert force-pushed the tox-configure branch from a9adbbc to 85cb196 Compare October 21, 2017 13:18

ryanhiebert requested a review from rpkilby October 21, 2017 13:30

ryanhiebert force-pushed the tox-configure branch from 85cb196 to 4b7358c Compare October 21, 2017 14:39

ryanhiebert mentioned this pull request Oct 21, 2017

Rename toxenv #80

Closed

4 tasks

ryanhiebert force-pushed the tox-configure branch from 4b7358c to ffdcb16 Compare October 21, 2017 23:20

rpkilby requested changes Oct 24, 2017

View reviewed changes

ryanhiebert mentioned this pull request Oct 25, 2017

Set toxenvs in tox_configure instead of during parsing #61

Closed

rpkilby approved these changes Oct 26, 2017

View reviewed changes

Merge branch 'master' into tox-configure

2e80f1f

ryanhiebert merged commit bc25fa2 into master Oct 28, 2017

ryanhiebert deleted the tox-configure branch October 28, 2017 00:43

ryanhiebert mentioned this pull request Oct 28, 2017

Counterintuitive passthru behavior #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move toxenv to tox_configure hook #78

Move toxenv to tox_configure hook #78

ryanhiebert commented Oct 17, 2017 •

edited

Loading

ryanhiebert commented Oct 17, 2017

ryanhiebert commented Oct 21, 2017

codecov-io commented Oct 21, 2017 •

edited

Loading

ryanhiebert commented Oct 21, 2017

ryanhiebert commented Oct 21, 2017

ryanhiebert commented Oct 23, 2017 •

edited

Loading

rpkilby left a comment

rpkilby Oct 24, 2017

ryanhiebert Oct 25, 2017

rpkilby Oct 25, 2017

rpkilby Oct 24, 2017

ryanhiebert Oct 25, 2017

ryanhiebert Oct 25, 2017

rpkilby Oct 26, 2017

ryanhiebert commented Oct 25, 2017

rpkilby left a comment

rpkilby Oct 25, 2017

rpkilby Oct 26, 2017


		# Make the envconfig for undeclared matched envs
		autogen_envconfigs(config, set(matched) - set(config.envconfigs))

Move toxenv to tox_configure hook #78

Move toxenv to tox_configure hook #78

Conversation

ryanhiebert commented Oct 17, 2017 • edited Loading

ryanhiebert commented Oct 17, 2017

ryanhiebert commented Oct 21, 2017

codecov-io commented Oct 21, 2017 • edited Loading

Codecov Report

ryanhiebert commented Oct 21, 2017

ryanhiebert commented Oct 21, 2017

ryanhiebert commented Oct 23, 2017 • edited Loading

rpkilby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhiebert commented Oct 25, 2017

rpkilby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhiebert commented Oct 17, 2017 •

edited

Loading

codecov-io commented Oct 21, 2017 •

edited

Loading

ryanhiebert commented Oct 23, 2017 •

edited

Loading