tox.ini rework: regroup things in the testenv section #1887

themr0c · 2019-03-28T09:02:01Z

This is a proposal to tidy up the tox configuration.

The rationale I followed was following: to avoid duplication of statements, regroup everything that can be regrouped in the testenv section. I hope this is also increasing readability of the tox.ini file.

I also moved the call to a second requirement file inside of the test-requiremenst.txt file.

I am not sure if this approach is consensual among tox users, so please tell me if this is going in the right direction, or absolutely not.

Signed-off-by: Fabrice Flore-Thebault [email protected]

PR Type

Feature Pull Request

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

decentral1se · 2019-03-28T12:55:07Z

Hmmm, not sure. I find the separate testenvs more readable? Not willing to stop initiative here if the others find this to be more useful though. Let's see and thanks for the efforts to improve this!

themr0c · 2019-03-28T13:22:13Z

@decentral1se that is exactly my point: have a discussion on the right direction to take. Experimenting here with "regrouping everything in testenv, tox.ini now under 100 loc".

tox.ini

tehsmyers · 2019-03-28T13:50:50Z

tox.ini

    functional: pytest test/functional/ {posargs}
    lint: flake8
    lint: yamllint -s test/ molecule/
+    metadata-validation: python -m setup checkdocs check --metadata --restructuredtext --strict --verbose
+    metadata-validation: twine check .tox/dist/*


I think that once deps or commands for a testenv break out to multiple lines, a dedicated testenv:section should probably be used. I love all the collapsed one-line dep/command testenvs though.

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

webknjaz · 2019-03-28T14:13:51Z

tox.ini

 passenv = *
 setenv =
    PYTHONDONTWRITEBYTECODE=1
 deps =
-    -rrequirements.txt


Please keep this line

webknjaz · 2019-03-28T14:32:55Z

tox.ini

@@ -1,44 +1,66 @@
 [tox]
 minversion = 3.7.0
 envlist =
+    build-docker


This is a list of envs executed when a user runs tox without env spec. I'd say that it should be a minimal needed thing for them to verify that a few main things work rather than wait for hours for all of the unneeded things to complete.
So they definitely don't need to check whether docker build works because in 99% cases they won't touch it.

If the focus is on the user here, would it make sense to limit this a little more, e.g.

envlist = doc format-check lint metadata-validation py{27,35,36,37}-ansible{25,26,27}-unit

?

Let's re-iterate:

Do users change docs often? Will they benefit on rebuilding docs all the time? Probably not.

format-check? It's probably worth it.

metadata-validation? Do they change packaging all the time? Probably not.

unit-tests? definitely yes.

functional tests? I think so.

Agreed on all counts. I left functional testing out because they don't "just work" for me without adding some pytest filtering. Functional testing should probably be in the default list of tests run, but not if those test runs require, say, working credentials for all drivers. I don't know if this is a common problem or only local to me, though.

webknjaz · 2019-03-28T14:33:29Z

tox.ini

@@ -1,44 +1,66 @@
 [tox]
 minversion = 3.7.0


let's bump to have the newest bugfixes and a few nice improvements..

Suggested change

minversion = 3.7.0

minversion = 3.8.3

Adding a requirement on tox version that was released 60 minutes ago is not safe.

Let things rest before bumping requirements to bleading edge releases.

I think there's no reason to distrust tox's release standards. Unless there's a specific problem that comes from using 3.8, we should feel free to track the latest tox. tox claims to follow semantic versioning, which we also have no reason to distrust. I'm not generally in favor of upgrading for upgrading's sake, but improvements to parallel testing in 3.8 would likely help get tox .. -p auto working.

Though, in line with the idea of incremental changes, it makes sense to only bump the tox minimums if tests are updated to require features introduced in higher versions.

It's only a patch release which fixed a minor bump. It doesn't matter when it was released, only proves that fixes get released fast.
And yes, tox's standards are high and current maintainers are very responsible.
The reason I want this update is that it ships certain features and bugfixes which I'd like to rely on.

webknjaz

To go for readability, we need to specify separate env vars, not dump everything into a single soup env...

Try separating things instead. it just breaks my brain when I try to understand what's going to happen in which cases...

What you've submitted is an antipattern. Check out tox.ini in tox's own config https://github.com/tox-dev/tox/blob/master/tox.ini or this one https://github.com/pypa/virtualenv/blob/master/tox.ini to get some inspiration.

ssbarnea

Too many changes inter-combined, making this change hard to review and merge. Lets do them one at a time.

ssbarnea · 2019-03-28T14:42:34Z

test-requirements.txt

@@ -1,3 +1,4 @@
+-r requirements.txt


I find this as an anti-pattern. Test requirements should not also include requirments.

While I find the refactoring of tox.ini very useful, I am afraid that the scope of the refactoring keeps growing and instead we should make smaller incremental improvements on it.

I don't think that it's an anti-pattern in general. It depends on the use-case. In this case, it's probably fine for now since I'm going to fix dependency management along with packaging anyway...

ssbarnea · 2019-03-28T14:45:16Z

tox.ini

@@ -1,44 +1,66 @@
 [tox]
 minversion = 3.7.0


Adding a requirement on tox version that was released 60 minutes ago is not safe.

Let things rest before bumping requirements to bleading edge releases.

tehsmyers · 2019-04-01T20:01:23Z

tox.ini

-
-[testenv:doc]
+[doc]
+# basepython can't be put in the testenv section


In my local testing (using tox >=3.8), this is not true. To have any effect, I need to put basepython inside the testenv:doc block for it to have any effect.

I did a test with tox 3.7.0 just now, and the basepython appears to be working as expected. The docs seem pretty clear that basepython belongs in testenv blocks:
https://tox.readthedocs.io/en/3.7.0/config.html#conf-basepython

The reason why I do not support bumping tox too fast is because tox is a system requirement which means that if a new version of tox is released that breaks its use on one python based project and molecule is forcing me to use that breading-edge new version I may endup with a case where I cannot run tox with both projects.

To avoid this case we only need to be carefult to wait at least two weeks before using a newer version of tox. I am confident about tox is tested well but nobody can guarantee that a new release does not break anything.

The same issue does not apply for most other test requirements because they are installed inside virtualenvironments managed by tox, so no risk of conflict.

Shortly, lets bump it when we need new features but after a cooldown period so we avoid getting burned ;)

tehsmyers · 2019-04-01T20:09:26Z

I'm going to attempt to sum up what's been discussed so far, please correct me if I missed something.

The preference appears to be to have explicit [testenv:*] blocks.
For consistency, existing one-command envs should probably be broken out into testenv blocks.
The default envlist should probably be format-check, lint, and py{27,35,36,37}-ansible{25,26,27}-{functional,unit}, based on the discussion in tox.ini rework: regroup things in the testenv section #1887 (comment). This effectively means leaving out doc by default and running the relatively inexpensive format-check before unit/functional tests.
Adding -r requirements.txt to test_requirements.txt is not preferred, specifying that explicitly via deps in the tox config is preferred.
Running functional testing is a pain right now, but @decentral1se's PR Improve supports_* driver checks (use Ansible logic) #1870 addresses those concerns and makes functional testing properly skip unsupported providers in local testing. Leaving functional testing in is good, in anticipation of Improve supports_* driver checks (use Ansible logic) #1870 being merged.
While bumping the tox minversion is not something to be afraid of, nothing in this PR requires tox 3.8 features (that I can see, at least), so it is left alone.

After applying all of those bullets to the current master tox.ini, here's the diff:

diff --git a/tox.ini b/tox.ini
index 3e1a5580..c8ac1c58 100644
--- a/tox.ini
+++ b/tox.ini
@@ -2,9 +2,8 @@
 minversion = 3.7.0
 envlist =
     lint
-    py{27,35,36,37}-ansible{25,26,27}-{functional,unit}
     format-check
-    doc
+    py{27,35,36,37}-ansible{25,26,27}-{functional,unit}
 skipdist = True
 skip_missing_interpreters = True
 
@@ -34,11 +33,6 @@ extras =
     windows
 commands_pre =
     find {toxinidir} -type f -not -path '{toxinidir}/.tox/*' -path '*/__pycache__/*' -name '*.py[c|o]' -delete
-commands =
-    unit: pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
-    functional: pytest test/functional/ {posargs}
-    lint: flake8
-    lint: yamllint -s test/ molecule/
 whitelist_externals =
     find
 # Enabling sitepackages is needed in order to avoid encountering exceptions
@@ -50,6 +44,9 @@ sitepackages = true
 [testenv:lint]
 deps =
     -rlint-requirements.txt
+commands =
+    flake8
+    yamllint -s test/ molecule/
 extras =
 skip_install = true
 usedevelop = false
@@ -72,7 +69,7 @@ usedevelop = false
 
 [testenv:doc]
 # doc requires py3 due to use of f'' strings and using only python3 as
-# basepython risks using python3.4 which is not supported.
+# basepython risks using python3.5, which does not support them.
 basepython = python3.7
 passenv = *
 commands =
@@ -119,3 +116,9 @@ commands =
     '
 whitelist_externals =
     sh
+
+[testenv:unit]
+commands = pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
+
+[testenv:functional]
+commands = pytest test/functional/ {posargs}

This results in a more focused diff that I think addresses the points discussed here.

I did throw in a quick comment change to the doc testenv basepython requirement to better reflect the current situation as I understand it.

webknjaz · 2019-04-01T20:58:00Z

@seandst you've perfectly summarized it. I just want to add that I've got #1895 to merge first which will address some of the concerns like the very existence of requirements.txt.

decentral1se · 2019-04-02T10:33:37Z

Running functional testing is a pain right now, but @decentral1se's PR #1870 addresses those concerns and makes functional testing properly skip unsupported providers in local testing. Leaving functional testing in is good, in anticipation of #1870 being merged.

Merged!

themr0c · 2019-04-02T14:38:52Z

I guess it's better to close this PR and do a new one with the changes proposed in the wrapup #1887 (comment)

themr0c · 2019-04-02T14:43:37Z

@seandst are you ready to propose a new PR based on your wrapup, or how should we proceed?

tehsmyers · 2019-04-02T18:12:50Z

#1870 was a major step in the right direction, so I'll keep an eye on #1895 and put up a new PR once those changes get settled.

tox.ini rework: regroup things in the testenv section

c3b0cbe

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

themr0c requested review from webknjaz, gundalow, ssbarnea and decentral1se March 28, 2019 09:02

themr0c added the test Improvement to quality assurance: CI/CD, testing, building label Mar 28, 2019

themr0c self-assigned this Mar 28, 2019

themr0c added 2 commits March 28, 2019 10:55

tox -e doc need skip_install = false and usedevelop = true

f1417d0

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

tox -e metadata-validation need skip_install = false

8c82a27

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

tehsmyers reviewed Mar 28, 2019

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

tehsmyers reviewed Mar 28, 2019

View reviewed changes

format-{check,enforce}

15af64d

Signed-off-by: Fabrice Flore-Thebault <[email protected]>

webknjaz reviewed Mar 28, 2019

View reviewed changes

tox.ini

passenv = *

setenv =

PYTHONDONTWRITEBYTECODE=1

deps =

-rrequirements.txt

Copy link

Member

webknjaz Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this line

webknjaz reviewed Mar 28, 2019

View reviewed changes

webknjaz suggested changes Mar 28, 2019

View reviewed changes

ssbarnea requested changes Mar 28, 2019

View reviewed changes

tehsmyers reviewed Apr 1, 2019

View reviewed changes

themr0c closed this Apr 2, 2019

tehsmyers mentioned this pull request Apr 18, 2019

Refactor tox envs #1995

Merged

tox.ini rework: regroup things in the testenv section #1887

tox.ini rework: regroup things in the testenv section #1887

Uh oh!

Conversation

themr0c commented Mar 28, 2019

PR Type

Uh oh!

decentral1se commented Mar 28, 2019

Uh oh!

themr0c commented Mar 28, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

ssbarnea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tehsmyers commented Apr 1, 2019

Uh oh!

webknjaz commented Apr 1, 2019

Uh oh!

decentral1se commented Apr 2, 2019

Uh oh!

themr0c commented Apr 2, 2019

Uh oh!

themr0c commented Apr 2, 2019

Uh oh!

tehsmyers commented Apr 2, 2019

Uh oh!

Uh oh!

webknjaz Mar 28, 2019 •

edited

Loading