Merge lp:~hopem/charm-helpers/lp1257763 into lp:charm-helpers

[Edward Hope-Morley]

Resubmitting with reviewer changes applied.

[James Page (james-page)]

Interestingly I start seeing errors when using tox - lists seem to be out of order...

[Edward Hope-Morley (hopem)]

Hmm dep versions need tweaking perhaps. Will dig...

[Edward Hope-Morley (hopem)]

Should be working better now, tested on Trusty. I had to apt-get build-dep a could of dependencies to get it to work tho (see commit message).

[Edward Hope-Morley (hopem)]

@jamespage quick thought, which version of tox are you using? I am using tox==1.6.1 since newer versions seem to flake out with openstack.

[Edward Hope-Morley (hopem)]

So, some more info. I have now tested this on precise, trusty, utopic and all work fine with tox==1.6.1 (incidentally the version used with openstack since never versions seem to break) but if I use latest i.e. tox==1.7.2 I get a bunch of unit test errors where lists have reversed order.

[Edward Hope-Morley (hopem)]

Adding in support dist-specific deps i.e. precise, trusty...

[Benjamin Saller (bcsaller)]

Thanks for this.

If you agree with the following I'm +1 on this.

=== modified file 'tox.ini'
--- tox.ini 2014-08-07 10:24:25 +0000
+++ tox.ini 2014-08-18 17:06:39 +0000
@@ -17,7 +17,7 @@

 [testenv:pep8]
 deps = -r{toxinidir}/test-requirements-trusty.txt
-commands = flake8 --ignore=E123,E501 {posargs}
+commands = flake8 --ignore=E123,E501 {posargs} charmhelpers

 [testenv:venv]
 commands = {posargs}

Currently it runs pep8 on the whole stdlib making its long and the feedback ignorable.

review: Needs Fixing

[Jorge Niedbalski (niedbalski)]

Hello,

I am more than happy to see this change set landing on charmhelpers. I will second the change proposed by @bcsaller.

With that fixed, LGTM.

review: Needs Fixing

[Cory Johns (johnsca)]

> Interestingly I start seeing errors when using tox - lists seem to be out of
> order...

I had proposed another tox merge with fixes for the fragile tests: https://code.launchpad.net/~johnsca/charm-helpers/tox/+merge/226180 (lines 157 through 266 in that diff).

However, that merge stalled because it contained wheel files for some of the dependencies, so maybe the test fixes could be ported to this MP and I could retire mine?

[Edward Hope-Morley (hopem)]

> > Interestingly I start seeing errors when using tox - lists seem to be out of
> > order...
>
> I had proposed another tox merge with fixes for the fragile tests:
> https://code.launchpad.net/~johnsca/charm-helpers/tox/+merge/226180 (lines 157
> through 266 in that diff).
>
> However, that merge stalled because it contained wheel files for some of the
> dependencies, so maybe the test fixes could be ported to this MP and I could
> retire mine?

Cory, I am concerned that the test errors (list inversions) caused by newer versions of tox are a result of tox causing some unexpected side effects in the tests, the scope of which is unclear. It is partially for this reason that I decided to pin tox to 1.6.1 which incidentally is the same rule currently used in upstream openstack.

[Edward Hope-Morley (hopem)]

> Thanks for this.
>
> If you agree with the following I'm +1 on this.
>
> === modified file 'tox.ini'
> --- tox.ini 2014-08-07 10:24:25 +0000
> +++ tox.ini 2014-08-18 17:06:39 +0000
> @@ -17,7 +17,7 @@
>
> [testenv:pep8]
> deps = -r{toxinidir}/test-requirements-trusty.txt
> -commands = flake8 --ignore=E123,E501 {posargs}
> +commands = flake8 --ignore=E123,E501 {posargs} charmhelpers
>
> [testenv:venv]
> commands = {posargs}
>
>
> Currently it runs pep8 on the whole stdlib making its long and the feedback
> ignorable.

Agreed, will fix. Thanks for reviewing.

[Cory Johns (johnsca)]

> Cory, I am concerned that the test errors (list inversions) caused by newer
> versions of tox are a result of tox causing some unexpected side effects in
> the tests, the scope of which is unclear. It is partially for this reason that
> I decided to pin tox to 1.6.1 which incidentally is the same rule currently
> used in upstream openstack.

The failures that I fixed in my merge are just tests relying on the ordering of of keyword args (i.e., dict items), which is undefined and subject to change at any time. My fixes just remove the strict ordering dependencies by replacing, e.g., assertEqual() with assertItemsEqual(). Without those changes, these tests are pretty much guaranteed to fail at some point.

I'm not averse to pinning the version of tox, but I think those test fixes should be made regardless.

[Corey Bryant (corey.bryant)]

For the most part this looks good to me and works well. I added a few minor suggestions inline. Also, is README.tests out of date? It seems like it could be updated or removed.

[Edward Hope-Morley (hopem)]

@corey.bryant - changes applied

@corey.johnsca - sounds like the newer versions of tox are just stricter in which case it is not a bug and you are absolutely correct. Could we possibly apply your suggested fixes as part of a separate patchset in order to avoid bloating this one? (if you agree i'll raise a seperate bug to get it fixed).

[Corey Bryant (corey.bryant)]

Sorry, a few more comments inline below.

[Cory Johns (johnsca)]

> @corey.johnsca - sounds like the newer versions of tox are just stricter in
> which case it is not a bug and you are absolutely correct. Could we possibly
> apply your suggested fixes as part of a separate patchset in order to avoid
> bloating this one? (if you agree i'll raise a seperate bug to get it fixed).

The problem is that the tox version only incidentally fixes the tests, because the ordering of dict items is arbitrary, even though it is mostly consistent. The docs (https://docs.python.org/2/library/stdtypes.html#dict.items) describe it as:

> CPython implementation detail: Keys and values are listed in an arbitrary
> order which is non-random, varies across Python implementations, and
> depends on the dictionary’s history of insertions and deletions.

Obviously, it's consistent enough that it hasn't been a big concern before now, but if we get these tests running under CI, as should be done, they will likely fail again at some point, even with the fixed version of tox.

That said, I'm fine with making the test fixes as part of a separate MP since they're not exactly related to the tox change and they pass most of the time. I'll go ahead and create the separate MP with just the test fixes.

[Edward Hope-Morley (hopem)]

@johnsca sure, let me know if/when you do this.

@corey.bryant i resubmitted this proposal after you added your last comments. I will address them now and resubmit.

[Stuart Bishop (stub)]

This branch has grown a pile of conflicts and alas needs to be reworked to support both Python 2 and Python 3.

review: Needs Resubmitting

Unmerged revisions

194. By Edward Hope-Morley on 2014-09-08

* allow DIST to be set in env
* added extra deps to README.test

193. By Edward Hope-Morley on 2014-09-01

applied corey.bryant changes

192. By Edward Hope-Morley on 2014-09-01

synced lp:charm-helpers

191. By Edward Hope-Morley on 2014-08-19

synced trunk

190. By Edward Hope-Morley on 2014-08-19

added specific dirs to pep8 tox run

189. By Edward Hope-Morley on 2014-08-07

added tox version check

188. By Edward Hope-Morley on 2014-08-07

pep8/lint should always be trusty

187. By Edward Hope-Morley on 2014-08-07

synced trunk and resolved merge conflicts

186. By Edward Hope-Morley on 2014-08-07

Added support for distribition specific dependencies

We now default to current LTS but can switch to other dist deps if needed.

185. By Edward Hope-Morley on 2014-07-30

* don't use site-packages
* set versions to trusty
* need to do this prior run:
    sudo apt-get build-dep python-launchpadlib
    sudo apt-get build-dep python-apt

=== modified file '.bzrignore'
--- .bzrignore	2014-06-09 14:56:35 +0000
+++ .bzrignore	2014-09-08 09:30:18 +0000
@@ -7,3 +7,4 @@
 .env/
 coverage.xml
 docs/_build
+.tox

=== modified file 'Makefile'
--- Makefile	2014-07-30 13:30:43 +0000
+++ Makefile	2014-09-08 09:30:18 +0000
@@ -1,7 +1,10 @@
-PROJECT=charmhelpers
 PYTHON := /usr/bin/env python
 SUITE=unstable
-TESTS=tests/
+# Default to current LTS. Supported envs are [precise,trusty].
+ifndef DIST
+        DIST=trusty
+endif
+TOXVER=$(shell pip show tox| grep "^Version:"| awk '{print $$2}')
 
 all:
 	@echo "make source - Create source package"
@@ -33,17 +36,25 @@
 	scripts/update-revno
 	python setup.py install --user
 
-test:
+# Newer versions of tox break the tests. Upstream Openstack also pin to this
+# version since newer versions seem to break backwards-compatibilty.
+assert_tox_ver:
+ifneq ($(TOXVER),1.6.1)
+	@echo "ERROR: tox==1.6.1 required (currently '$(TOXVER)') - try 'sudo pip install tox==1.6.1'"
+	exit 1
+endif
+
+test: assert_tox_ver
 	@echo Starting tests...
-	@$(PYTHON) /usr/bin/nosetests -s --nologcapture tests/
+	@tox -epy27-$(DIST) -- -s
 
-ftest:
+ftest: assert_tox_ver
 	@echo Starting fast tests...
-	@$(PYTHON) /usr/bin/nosetests --attr '!slow' --nologcapture tests/
+	@tox -epy27-$(DIST) -- --attr '!slow'
 
-lint:
+lint: assert_tox_ver
 	@echo Checking for Python syntax...
-	@flake8 --ignore=E123,E501 $(PROJECT) $(TESTS) && echo OK
+	@tox -epep8 
 
 docs:
 	- [ -z "`dpkg -l | grep python-sphinx`" ] && sudo apt-get install python-sphinx -y

=== modified file 'README.test'
--- README.test	2014-07-30 13:30:43 +0000
+++ README.test	2014-09-08 09:30:18 +0000
@@ -1,12 +1,12 @@
 Required Packages for Running Tests
 -----------------------------------
-python-shelltoolbox
-python-tempita
+make
 python-nose
-python-mock
-python-testtools
-python-jinja2
-python-coverage
-python-netifaces
-python-netaddr
+python-pip
+tox==1.6.1
+python-dev
+libapt-pkg-dev
 zip
+
+NOTE: the first time tests are run it is usually necessary to install some build
+      dependencies for some of the test-requirements (apt-get build-dep).

=== added file 'test-requirements-precise.txt'
--- test-requirements-precise.txt	1970-01-01 00:00:00 +0000
+++ test-requirements-precise.txt	2014-09-08 09:30:18 +0000
@@ -0,0 +1,15 @@
+coverage==3.7.1
+launchpadlib==1.10.2
+mock==1.0.1
+nose==1.1.2
+PyYAML==3.10
+python-apt==0.8.5
+simplejson==3.3.1
+testtools==0.9.35
+Tempita==0.5.1
+bzr+http://bazaar.launchpad.net/~yellow/python-shelltoolbox/trunk@17#egg=shelltoolbox
+netifaces==0.8
+netaddr==0.7.10
+bzr==2.6.0
+Jinja2==2.6
+flake8==2.1.0

=== renamed file 'test_requirements.txt' => 'test-requirements-trusty.txt'
--- test_requirements.txt	2014-07-09 15:58:35 +0000
+++ test-requirements-trusty.txt	2014-09-08 09:30:18 +0000
@@ -1,21 +1,15 @@
-# If you don't have the required packages on your system and prefer
-# to keep your system clean, you can do the following to run the
-# charm-helper test suite:
-# $ virtualenv .env
-# $ . .env/bin/activate
-# $ pip install -r test_requirements.txt
-# NOTE: python-apt is compiled and will require libapt-pkg-dev for headers.
-# Then `make test` should find all its dependencies.
-coverage==3.6
+coverage==3.7.1
 launchpadlib==1.10.2
 mock==1.0.1
 nose==1.3.1
 PyYAML==3.10
-python-apt
-simplejson==3.3.0
-testtools
-Tempita==0.5.1
+python-apt==0.8.5
+simplejson==3.3.1
+testtools==0.9.35
+Tempita==0.5.2
 bzr+http://bazaar.launchpad.net/~yellow/python-shelltoolbox/trunk@17#egg=shelltoolbox
-http://alastairs-place.net/projects/netifaces/netifaces-0.6.tar.gz
+netifaces==0.8
+netaddr==0.7.10
 bzr==2.6.0
 Jinja2==2.7.2
+flake8==2.1.0

=== added file 'tox.ini'
--- tox.ini	1970-01-01 00:00:00 +0000
+++ tox.ini	2014-09-08 09:30:18 +0000
@@ -0,0 +1,23 @@
+[tox]
+# Default to current LTS
+envlist = pep8,py27-trusty
+skipsdist = True
+
+[testenv]
+install_command = pip install --allow-unverified launchpadlib --allow-unverified lazr.authentication --allow-unverified python-apt {opts} {packages}
+commands = nosetests --nologcapture {posargs:tests} tests
+
+[testenv:py27-precise]
+basepython = python2.7
+deps = -r{toxinidir}/test-requirements-precise.txt
+
+[testenv:py27-trusty]
+basepython = python2.7
+deps = -r{toxinidir}/test-requirements-trusty.txt
+
+[testenv:pep8]
+deps = -r{toxinidir}/test-requirements-trusty.txt
+commands = flake8 --ignore=E123,E501 {posargs} charmhelpers tests
+
+[testenv:venv]
+commands = {posargs}