Skip to content
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

tox 3.7.0 doesn't fail on undefined factor (where 3.6 did) #1160

Closed
medmunds opened this issue Feb 9, 2019 · 3 comments
Closed

tox 3.7.0 doesn't fail on undefined factor (where 3.6 did) #1160

medmunds opened this issue Feb 9, 2019 · 3 comments
Labels
bug:normal affects many people or has quite an impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. pr-merged

Comments

@medmunds
Copy link

medmunds commented Feb 9, 2019

In tox 3.6 and earlier, if your tox.ini delcares an envlist, running tox -e py36-someUndefinedFactor would exit with "ERROR: unknown environment 'py36-someUndefinedFactor'" (and without creating a venv).

In tox 3.7.0, the same command seems to ignore "someUndefinedFactor", and instead creates a venv and attempts to run the commands in it.

This makes it difficult to catch typos in commands or tox.ini.

Reproduction

(I'm on Python 3.7.1, but don't think this matters. Full system info below.)

  1. Create an empty project with this tox.ini. It must include an envlist to reproduce this bug. Notice the envlist doesn't cover "django22":
[tox]
envlist =
    py{36,37}-django{20,21}

[testenv]
deps =
    django20: django~=2.0.0
    django21: django~=2.1.0
skip_install = true
commands =
    python -c 'import django; print(django.__version__)'
  1. pip install tox==3.6.1

  2. Run tox -e py36-django22 (django22 is not in the envlist).
    Results: "ERROR: unknown environment 'py36-django22'" (as expected)

  3. pip install tox==3.7.0

  4. Run tox -e py36-django22 again.
    Expected: same unknown environment error as with tox 3.6.1.
    Actual results: tox attempts to create the environment and run the commands in it:

py36-django22 create: .../.tox/py36-django22
py36-django22 run-test-pre: PYTHONHASHSEED='1187325010'
py36-django22 runtests: commands[0] | python -c 'import django; print(django.__version__)'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'django'
ERROR: InvocationError for command '.../.tox/py36-django22/bin/python -c import django; print(django.__version__)' (exited with code 1)

System info

macOS 10.14.2
Python 3.7.1
pip list

Package    Version
---------- -------
filelock   3.0.10
pip        19.0.2
pluggy     0.8.1
py         1.7.0
setuptools 40.8.0
six        1.12.0
toml       0.10.0
tox        3.6.1
virtualenv 16.4.0
wheel      0.32.3
medmunds added a commit to anymail/django-anymail that referenced this issue Feb 9, 2019
* Use TOX_FORCE_IGNORE_OUTCOME env var to implement equivalent of
  tox-travis unignore_outcomes
* Pin tox to ~3.6.1 to ensure unknown toxenv fails build
  (tox-dev/tox#1160)

* Also, pin detox to 0.18 (avoids crash on exit as detox attempts
  to warn that it's deprecated)
@gaborbernat gaborbernat added bug:normal affects many people or has quite an impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Feb 9, 2019
@medmunds
Copy link
Author

medmunds commented Feb 10, 2019

I believe I've located the change that introduced the problem. It was part of PR #1102 that implemented parallel invocation.

Here's what seems to be happening:

  • session.Session._makevenv will correctly issue an error if the requested env name is not found in config.envconfigs (in both 3.6.1 and 3.7.0).
  • config.envconfigs is built in ParseIni.__init__ from all_envs whose names include only known_factors (or some other conditions that aren't relevant to this issue). This has not changed between 3.6.1 and 3.7.0.
  • What has changed is the calculation of known_factors. In 3.6.1 factors are extracted from the stated_envlist retrieved directly from config file. In 3.7.0 that was changed to extract factors from config.envlist, which is initialized earlier and includes not only the config file's envlist but also any envs requested on the command line or TOXENV.

So in 3.7.0, the known_factors are extracted from the requested command-line env, and as a result the requested env will always be created, because it consists entirely of (its own!) known factors.

@gaborbernat I can't really tell if this was something you needed to support parallel execution, or if maybe it was just a typo or attempted cleanup. (It took me a while in a debugger to figure out that config.envlist is not in fact the "envlist" from the config file.)

A quick fix might be to revert that one changed line, but I haven't checked to see if that breaks anything else. A better fix might be to rename config.envlist to something like config.envs_to_run, but that would touch a lot of code.

@gaborbernat gaborbernat changed the title Tox 3.7.0 doesn't fail on undefined factor (where 3.6 did) tox 3.7.0 doesn't fail on undefined factor (where 3.6 did) Feb 11, 2019
@gaborbernat
Copy link
Member

@medmunds thanks for getting to the bottom of this. This was more a way to clean up the code, an unfortunate regression. I would say let's try to revert that line and add a test to defend against such regression. Sadly we can't rename things yet because of backward compatibility with our plugins. 😭 Can you create a PR for this? thanks!

@medmunds
Copy link
Author

@gaborbernat done.

Incidentally, the existing tests for the "unknown environment" error didn't catch this, because the problem only occurs if tox.ini includes an envlist. I'll update the bug description.

gaborbernat pushed a commit that referenced this issue Feb 11, 2019
…#1165)

* Fix missing error for `tox -e unknown` when tox.ini declares envlist.

Fixes #1160.

* fix_lint (not just fix-lint)
@helpr helpr bot added pr-merged and removed pr-available labels Feb 11, 2019
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug:normal affects many people or has quite an impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. pr-merged
Projects
None yet
Development

No branches or pull requests

2 participants