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
Update testing matrix #20
Conversation
PR updated to fix test failures. hawkrest currently doesn't work under Django 1.9, so I've marked those jobs as being allowed to fail. In a later PR I'll fix the issues and remove that annotation. |
@@ -1,30 +1,23 @@ | |||
sudo: false | |||
language: python | |||
python: | |||
- "2.7" | |||
- "3.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we still need to list each individual version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if you saw the commit message for that change? :-)
https://github.com/edmorley/hawkrest/commit/ecddb7df8de3813b2b6d352bb0018acd567d648e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tox controls the Python versions tested, so we only need one Python
version specified at the Travis level
This isn't entirely true. I think what you mean is that tox controls the virtualenv for each Python. The machine still needs each version of Python installed otherwise tox won't be able to find an executable for it. I think the tests were working before (with the duplicate entry typo) because travis makes some versions available by default. I'd rather see those versions declared explicitly for when travis removes old versions in the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unfortunately won't work. Every entry under the 'python' key creates another matrix of tests. If I added both Python 3.5 and 2.7 here we'd run the tests twice, duplicating them (since both Travis and Tox would be doing all the permutations). See:
https://docs.travis-ci.com/user/customizing-the-build/#Build-Matrix
(And yeah Travis makes python2.7, python3.4 available by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clearer: we can either control the python version at the Travis level, or the tox level, but not both. I'm open to doing either :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, weird! Can you just put a comment above this line then? Something like "we aren't explicitly declaring all versions needed by tox because ...". I think this is a weird assumption by Travis. A comment will prevent someone from trying to fix it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language versions are intended to be used for a testing matrix, rather than "these are the list of versions I want installed in the same image", but I agree sometimes the Travis testing matrix isn't very flexible. Treeherder has to go to some great contortions to get the exact environment we need for each test suite (we abuse the matrix include option quite horrifically: link).
What did you mean by the test matrix is too big? You mean for local development? When developing, I always run with I'd like to keep at least 1.7 in here. I know support has been dropped but it will be in use at Mozilla for a long time. Actually, a lot of our apps are still on 1.6. When Django drops support they mostly mean they won't take in additional pull requests. Obviously if there is a dramatic security bug, Mozilla will have to scramble to upgrade so it's not ideal. It can take a lot of effort to migrate major versions of Django for a large app though. |
Both Django 1.6 and 1.7 are out of the extended support period, meaning it's not just a case of them not accepting pull requests, but there are no security updates either. Django 1.8 is their long-term release branch, for which they've committed to providing security updates until at least April 2018. By "too large" I mean that 3 versions of Python x 4 versions of Django x 4 versions of django-rest-framework = 48 jobs on Travis, which seems a bit excessive. In addition, new feature work has to potentially work around issues with older (and unsupported) versions of libraries. It's up to you - I can add them back in if you'd prefer (perhaps with not the full matrix - eg only test on Python 2.7 for older versions), it's just that many other projects have already dropped support for them. |
Yeah, I'd like to see just one matrix: Python 2.7 + Django 1.7 + the latest version of DRF just as a sanity check. We can't break support for it because of the other Mozilla apps using hawkrest.
Yes, I think it should. However, if something comes up in new feature work that makes it hard to support an older version then we can consider dropping support. |
@@ -4,17 +4,15 @@ | |||
# When updating the envlist, be sure to also update TOX_ENV in .travis.yml | |||
envlist = | |||
docs, | |||
{py27,py34}-django{1.6,1.7,1.8}-drf{3.0,3.1,3.2} | |||
{py27,py34,py35}-django{1.8,1.9}-drf{3.2,3.3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add a single combo as I mentioned earlier, you would add a new line to this section, like:
py27-django1.6-drf3.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, do you want Django 1.7 or both 1.6 and 1.7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(django-rest-framework 3.3 doesn't support Django 1.6)
Tox controls the Python versions tested, so we only need one Python version specified at the Travis level. This is effectively a no-op, since Travis was already ignoring the "3.4" entry due to the duplicate "python" key.
Since there have been two newer major release branches since (3.2, 3.3), and 3.0/3.1 are no longer receiving updates: https://github.com/tomchristie/django-rest-framework/releases
We don't test on Python 2.6 or 3.3, so removing them from the classifiers. However, we do test on Python 3.4, so adding that instead.
Pinning to specific versions is only going to hide inadvertent incompatibilities with new point releases.
Both mainstream and extended support has now ended for Django 1.6/1.7: https://www.djangoproject.com/download/#supported-versions Now that we're shortly going to test on Python 3.5, Django 1.9 and django-rest-framework 3.3, the testing matrix is going to get very large unless we cut back on testing for EOL Django versions. It's unlikely people stuck on older versions of Django are using Python 3, so this seems like an easy win.
Test django-rest-framework 3.3 against Django 1.7/1.8. (3.3 dropped support for Django 1.6)
There are currently test failures when testing against Django 1.9, so this enables testing, but marks the jobs as allowed to fail until they are fixed. Django 1.9 support was only added to django-rest-framework in v3.3: encode/django-rest-framework@3d7740f ...so unlike the other Django versions, Django 1.9 cannot be tested against django-rest-framework v3.2.
Python 3.5 has to be installed on-demand with the current Travis images, so we must set the base Python version to 3.5. Tox will still be able to use the installed-by-default Python 2.7 and 3.4 for the appropriate jobs in the test matrix.
PR updated, preserving testing on Django 1.6 and 1.7, but testing on fewer combinations of Python/django-rest-framework. |
looks great, thanks |
Thank you :-) |
Adds testing for:
Drops testing for:
See individual commit messages for more details :-)