-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move toxenv to tox_configure hook #78
Conversation
ad81d66
to
c7f83b1
Compare
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. |
c7f83b1
to
08b8017
Compare
08b8017
to
a9adbbc
Compare
Due to the change of setting the |
a9adbbc
to
85cb196
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I finally got the bugs worked out, and this is ready for review! |
85cb196
to
4b7358c
Compare
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.
4b7358c
to
ffdcb16
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, just looking for clarification.
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what was the reason behind this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense.
|
||
# Make the envconfig for undeclared matched envs | ||
autogen_envconfigs(config, set(matched) - set(config.envconfigs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@rpkilby : If you're satisfied with my responses, can you update your review to an approval? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all makes sense to me now, thanks for the clarification.
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense.
|
||
# Make the envconfig for undeclared matched envs | ||
autogen_envconfigs(config, set(matched) - set(config.envconfigs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.