Remove 3.5 and xenial support (SC-711)#1167
Conversation
TheRealFalcon
changed the title
| should be placed before the value under test: | ||
| ``assert expected_value == function_under_test()`` | ||
|
|
||
|
|
There was a problem hiding this comment.
I snuck this removal in here as I was modifying this file as this rule seems arbitrarily restrictive and one I purposely ignore all the time 🙂
If anybody disagrees, I can put it back in.
TheRealFalcon
changed the title
| reproduced by running:: | ||
|
|
||
| py.test-3 --fixtures -q | grep "^[^ -]" | grep -v '\(no\|capturelog\)' | sort | sed 's/.*/* ``\0``/' | ||
| python3 -m pytest --fixtures -q | grep "^[^ -]" | grep -v '\(no\|capturelog\)' | sort | sed 's/.*/* ``\0``/' |
There was a problem hiding this comment.
A few comments on this one-liner:
Presumably the "no" is to remove no tests ran in 0.00s, but this filters out no_cover on more recent pytest versions - future-proof with longer match string.
"capturelog" doesn't filter anything on pytest 6.2.5, I assume this is something old that isn't needed?
It looks like " [session scope]" is being manually removed after running the command. This can be updated with a small updated to the sed command.
Assuming others agree with the above comments, this is what I propose:
python3 -m pytest --fixtures -q | grep "^[^ -]" | grep -v 'no tests ran in' | sort | sed 's/ \[session scope\]//g;s/.*/* ``\0``/g'
There was a problem hiding this comment.
Works for me, thanks!
TheRealFalcon
force-pushed
the
remove-3.5
branch
from
53a12e3 to
7dc74ca
Compare
holmanb
left a comment
holmanb
left a comment
There was a problem hiding this comment.
LGTM
Related: We still have a couple of comments saying "todo: remove when 2.7 is no longer supported" in the codebase. When we get around to removing 3.5-specific code, we may want to keep those in mind as well :)
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Hey @TheRealFalcon this looks really good, only comments I have are some of the tox refactors as I think we need to be able to support running these targets on Bionic, Focal and Impish. I think we still need a dev target that pins a different httpretty that the straight lowest-supported testenv. Also we need to make sure tox -e lowest-supported works on Bionic directly right?
Here's a diff of what I think works for my inline comments:
diff --git a/tox.ini b/tox.ini
index d57111331..1a6a441d2 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
[tox]
-envlist = py3, lowest-supported, flake8, pylint, black, isort
+envlist = py3, lowest-supported-dev, flake8, pylint, black, isort
recreate = True
[testenv]
@@ -54,7 +54,7 @@ commands = {envpython} -m pytest \
{posargs:--cov=cloudinit --cov-branch \
tests/unittests}
-[testenv:lowest-supported]
+[lowest-supported-deps]
# Tox is going to install requirements from pip. This is fine for
# testing python version compatibility, but when we build cloud-init, we are
# building against the dependencies in the OS repo, not pip. The OS
@@ -73,11 +73,26 @@ deps =
jsonschema==2.6.0
netifaces==0.10.4
# test-requirements
- httpretty==0.8.14
pytest==3.3.2
pytest-cov==2.5.1
- attrs==17.4.0 # Needed by pytest and default causes failures
+ # attrs==17.4.0 # Needed by pytest and default causes failures
+ attrs=17.4.0
+
+[testenv:lowest-supported]
commands = {[testenv:py3]commands}
+deps =
+ {[lowest-supported-deps]deps}
+ # Official lowest-supported deps released in bionic-updates
+ httpretty==0.8.14
+
+[testenv:lowest-supported-dev]
+commands = {[testenv:py3]commands}
+deps =
+ {[lowest-supported-deps]deps}
+ # httpretty in bionic is 0.8.14, not 0.9.5. The oldest version to work with
+ # Python 3.7+ is 0.9.5, because it is the first to include this commit:
+ # https://github.com/gabrielfalcao/HTTPretty/commit/5776d97da3992b9071db5e21faf175f6e8729060
+ httpretty==0.9.5
[testenv:doc]
deps =| [testenv:py27] | ||
| basepython = python2.7 | ||
| deps = -r{toxinidir}/test-requirements.txt | ||
| [testenv:lowest-supported] |
There was a problem hiding this comment.
I don't think we can do away with a shared lowest-supported-deps definition just yet.
Since lowest-supported is included in our default envlist it'll be run automatically in developer environments via tox -p auto. Without continuing to "pin" httpretty=0.9.5 in a lowest-supported-dev target we'll fail to pass locally on focal, hirsute and I think impish too.
It'd like to ensure developers can run this build env to avoid getting a suprirse if CI runs it and fails. What do you think?
| httpretty==0.8.14 | ||
| pytest==3.3.2 | ||
| pytest-cov==2.5.1 | ||
| attrs==17.4.0 # Needed by pytest and default causes failures |
There was a problem hiding this comment.
Inline comments still break on bionic tox. This comment needs to be a separate line otherwise we'll hit something like:
ERROR: Invalid requirement: 'attrs==17.4.0 # Needed by pytest and default causes failures'
|
Note I discovered a potentially related set of failures with running |
Includes: - Update tox.ini and .travis.yml accordingly - Cleanup tox.ini with new tox syntax and cloud-init dependencies - Update documentation accordingly - Replace/remove xenial references where additional testing isn't required - Remove xenial checks in integration tests - Replace yield_fixture with fixture in pytest tests Sections of code commented with lines like "Remove when Xenial is no longer supported" still exist as they're require additional testing.
TheRealFalcon
force-pushed
the
remove-3.5
branch
from
842dc59 to
30cb947
Compare
|
Really good comments @blackboxsw , thanks. I never knew why we had so many xenial definitions in our tox definition, and now I do 😄 . Updated based on comments. |
Put up the related PR to avoid leaks detecting a valid LXD datasource in test_ds_identify. #1178 |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
+1! Bring it home! Bye bye xenial.
3 participants
Proposed Commit Message
Additional Context
I created a separate ticket for dealing with code that would need additional testing.
Test Steps
Checklist: