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

Generative environments are only created if the fragment is used #191

Closed
pytoxbot opened this Issue Sep 17, 2016 · 16 comments

Comments

Projects
None yet
1 participant
@pytoxbot

pytoxbot commented Sep 17, 2016

Example:

[tox]
envlist = py27-{a,b}

[testenv]
deps =
   b: pytest
commands = true

Makes tox -l complain: ERROR: unknown environment 'py27-a'

My current workaround is to conditionally set some bogus env variable (which let me run int o #190 :)).

Having unused fragments is useful, I do it for example in https://github.com/hynek/structlog/blob/master/tox.ini to be more explicit and save myself listing ‘bare’ environments.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

add changelog: fix issue191: lessen factor-use checks

→ <<cset 0a85dfb46090>>

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

Lessen factor use check

Fixes issue 191

→ <<cset 81587bb49a77>>

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

I implemented less strict behaviour: envlist factors are not checked to be used in testenv, command line and environment vars checked against both envlist and testenv. This way at least cmdline typos will be caught.

https://bitbucket.org/hpk42/tox/pull-request/130/lessen-factor-use-check/diff

Dups check will go either separate or into unknown future.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

Holger, current check is about known_factors not being used, not the way you described it. It is allowed now to have factors not referred in envlist and it always was allowed to have testenvs not listed.

@pytoxbot pytoxbot closed this Sep 17, 2016

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

In principle i think it's a good idea to verify that test environments are not configured in exactly the same way and give a warning in that case. If for some reason the test commands depend on the environment name then you would need to set e.g. a random environment variable. I don't know how hard it is to implement such a check currently.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @carljm

Sure, I see how relaxing this check could allow a mistake to pass unnoticed, but I think it is worse to make valid cases hard to express.

I would also be fine with leaving the existing check, but making it a warning instead of an error.

This is Python, we are consenting adults. Warnings about possible mistakes are fine, but preventing tox from running with a configuration that expresses a reasonable use case is bad.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

Quite surely the PR can be improved.

The main thing is that semantically/UI wise i'd like "envlist" to be the definitive source of environments along with any -e command line specified. tox -l and tox --showconfig should list all such environments. This might limit our ability to do verification because there might be factors that are only used through a tox -e command so we can't currently reliably complain about them appearing in tox.ini. I guess that's a rare case, though, and we could thus require a new option extra_factors listing all factors which can additionally appear. To summarize this would mean for verification::

  • known_factors = envlist-factors + extra-factors
  • used_factors = factors used in all testenv expressions
  • complain about used factors that are not in known factors

Not sure how crucial the verification is, though, so maybe we could just not do verification in tox-1.9 and see how that goes.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

Anyway we need to decide whether we want the check. @hpk42?

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

Several names for identical envs is definitely an error as it will make tox call make twice the same work. Sure, it could be used as dump aliases, but I am in favor of preventing this.

The problem is such heuristics can spare an error that could have been caught by stricter one. See how pg is skipped in testenv here:

[tox]
envlist = py{26,27}-{sqlite,mysql,pg}

[testenv]
setenv =
   sqlite: DB=SQLITE
   mysql: DB=MYSQL
commands = 
    py.test

A code could fallback for DB not set to e.g. sqlite and still work, while leaving PostgreSQL not tested.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @carljm

I see. I think that with generative environments the old version of the check is now too strict, since there are plenty of valid cases to have an env factor whose only distinguishing characteristic is the absence of some configuration that is present in other factors.

If the "no identical envs" check is not hard/complex to implement, I don't see anything wrong with it (though I think perhaps it should be a warning rather than an error), but I also don't see it as necessary.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

The confusion stems from a phrase "unknown environment X" being ambiguous. Tox means there is no [testenv] section for X (in either [testenv:X] or [testenv] with X factor used), but it could be easily understood as there is no X in envlist.

Tox always was checking that every env in envlist has its section. Starting from 1.8 it also looks for root testenv having all the factors. Both measures a directed at user not forgetting to describe all the environments. However, generative testenvs introduce additional confusion, since you can always generate a testenv from root one, even if it doesn't contain all the factors. Holgers fix completely strips off this check (while leaving some now meaningless code). So it's design decision to make: do we want to keep the check? If yes, then we need to write better error message, which is I was proposing. If not we need to remove all "factor is used" code.

A new thought. We can try to make check more intelligent. Like make it an error if some differently named envs produce same testenvs. This way we will probably catch most of errors not showing themselves during normal tox run. Not sure how reliable this will be.

envlist = py27-foo{1,2}{,-b} not working is a separate bug.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @carljm

Also, FWIW, the first proposed workaround breaks when combined with another factors set, I think because it puts the separator inside a factor (which also makes the use of that factor uglier). I tried to do the equivalent of this:

[tox]
envlist = py27-foo{1,2}{,-b}

[testenv]
deps =
   b: pytest
   foo1: foo1
   foo2: foo2
commands = true

Which results in:

$ tox -l
ERROR: unknown environment ''

This is fixable by changing the order to envlist = py27{,-b}-foo{1,2}, but I still think it's problematic as a general solution.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @carljm

Alex: could you explain why the fix Holger pushed is problematic, and why you see the current behavior as preferable?

I just ran into this issue, and I think the current behavior is very unintuitive and surprising. If I have defined an environment in the envlist (that includes a generative environment), it should exist, regardless of whether its factors are all referenced elsewhere or not. The envlist setting should be in full command of which environments exist.

Also, the error raised from tox -l is quite confusing: I have obviously defined the environment py27-a in my envlist, so why would tox be telling me it's unknown? If the current behavior must be kept for some reason, a clearer error message is needed.

It's good to know there are workarounds available (I didn't even realize {,-b} was a valid factors set), but I think those workarounds are not obvious, and the intuitive choice is to make envlist authoritative.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

actually this issue is not resolved -- a changeset was only pushed to a branch.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @Suor

This could be written as:

[tox]
envlist = py27{,-b}

[testenv]
deps =
   b: pytest
commands = true

Or as:

[tox]
envlist = py27,py27-b
...

The fix you added are problematic. And an error from tox -l is intentional. Maybe error message could be improved but I am for keeping current behavior.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

fix issue191: factors are now defined by the "envlist" and
you don't need to use them in the testenv section anymore.

→ <<cset 9aae06bf4a78>>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment