Code review comment for lp:~stub/charms/precise/postgresql/integration

Revision history for this message
Stuart Bishop (stub) wrote :

'make lint' is clean when run on my trusty box. I did merge in your branch, but it introduces lint failures:

stub@belch:~/charms/postgresql/integration$ make lint
Lint check (flake8)
directory hooks
checking hooks/helpers.py
checking hooks/hooks.py
hooks/hooks.py:991:80: E501 line too long (80 > 79 characters)
hooks/hooks.py:998:80: E501 line too long (80 > 79 characters)
checking hooks/test_hooks.py
directory testing
checking testing/__init__.py
checking testing/jujufixture.py
directory tests
checking test.py
make: *** [lint] Error 1

I suspect you are running from Precise, with older versions of the lint checkers? Or a more modern version? Possibly more modern since one of the comments changed predates my maintainership. Apart from the trivially fixed long lines, your other changes look benign so I've merged it in anyway.

relation-list does work with 'juju run', because 'juju run' runs the command in a hook context (and is why it is functionally different to juju ssh). I do notice you are running juju 1.18, and I'm running the test suite with juju 1.20 (latest stable). I am not aware of any 1.20 features or bug fixes that I'm relying on, so 1.18 could be fine.

JUJU_REPOSITORY needs to be set, yes. I think we are stuck with this until I can port the tests to Amulet. Or when juju lets me deploy the cwd as a charm rather than requiring this baroque structure.

The tests are flaky, and will alas remain flaky until Bug #1200267 is addressed. I can increase the various sleeps scattered around the code base to make things better with slower providers, which will of course make the test suite even slower. I have increased what I think is the relevant timeout, and am now ignoring all failures from 'juju run' in this section (rather than just errcode==2, which is what we usually see when juju is caught with its pants down) - I think there was a race here where I might attempt to list units in a relation before the relation has been joined and an error code returned (the juju run failures you see).

The test_failover failure is interesting. I think for you it was failing just due to not waiting long enough. For me, I think that a race condition has been introduced with juju 1.20. I need to investigate this further.

« Back to merge proposal