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

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

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

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).

@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.

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.

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

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.

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.

> 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?

> > 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.

> 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, 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.

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.

@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).

Sorry, a few more comments inline below.