Counterintuitive passthru behavior

[Suor]

When nothing matched tox-travis for whatever reason runs all tox envs. Here is an example - there is no pypy-djmaster env in envlist.

One would expect either TOXENV=pypy-djmaster tox be run anyway or empty run. Full run is quite surprising.

[ryanhiebert]

I agree that this is unexpected and wrong behavior. It appears that this relates to how Tox itself deals with a set, but empty, TOXENV environment variable, which is what we're using to tell Tox which envs to run.

Here's an example of what I see on a project of mine:

$ TOXENV='py36' tox -l
py36
$ TOXENV='' tox -l
py36
docs

I don't think this is bad behavior from Tox, but it is behavior that we want to override in Tox-Travis. What we want to do is add a check so that it will exit with an error message and error code immediately when this happens. This is where that environment variable is set:

https://github.com/ryanhiebert/tox-travis/blob/master/src/tox_travis/toxenv.py#L41

I can't imagine that anyone is actually relying on this behavior to do what they are intending. Still, this may cause test suites that are currently passing to fail. Does anyone have any input on the Right Way to roll out this fix? @jayvdb @rpkilby

[ryanhiebert]

The reason that I don't want to automatically run pypy-djmaster in this case is because (a) that env is likely to be unconfigured in tox.ini, and (b) Even if it were configured, there may be other factors, not involved in the .travis.yml, that also need to be considered. For instance, two similar environment runs, one with an optional module, and one without, and it may be best in that case to have those two tox envs run on the same Travis job.

---

While I would rather have this throw an error, I think that perhaps the best approach for backward compatibility is to log a warning, and then exit as a success for now. This avoids the issue of breaking working builds, while also not running unintended envs.

I think it would be reasonable, when implementing #58, to have this become a part of strict matching mode, to cause errors when this likely misconfiguration is detected. Then this would become part of a possible future breaking change (1.0?) to switch the default to strict matching mode.

[rpkilby]

While I would rather have this throw an error, I think that perhaps the best approach for backward compatibility is to log a warning, and then exit as a success for now.

I'm generally in favor of backwards compatibility, but I'm not too concerned about bending over backwards in this case. I honestly doubt that anyone is relying on this bug as a legitimate behavior. If builds are accidentally broken by this change, then that likely indicates that the build was erroneously succeeding.

Given that the package is still a beta, it would seem reasonable to simply default to "strict" matching.

[ryanhiebert]

My feeling is that we get enough benefit from the non-breaking change that the incentive just isn't there to go ahead an break it. When we release another breaking change, then I'll be for causing this to break as well at that time. I appreciate you weighing in, @rpkilby, and I really appreciate having you be part of this team.

[rpkilby]

I'm responding on the other thread, but I'm reversing my position. I don't think it makes sense to change the default now, given that there are legitimate use cases for the existing behavior.

[ryanhiebert]

The solution I mentioned earlier (silently passing, but not running any envs) has been implemented as a side-effect of the refactor in #78.