[ckan-dev] test failures

Fri Feb 4 10:40:13 UTC 2011

Hi,

On 4 February 2011 09:04, David Read <david.read at okfn.org> wrote:
>
>
> On 3 February 2011 22:35, David Raznick <kindly at gmail.com> wrote:
>>
>>
>> On Thu, Feb 3, 2011 at 9:50 PM, David Read <david.read at okfn.org> wrote:
>>>
>>> Hi all,
>>>
>>> I've changed buildbot to run ckan tests twice now: firstly with sqlite
>>> and then again with postgres in the same buildbot script (I should split it
>>> into two tasks, but that is for another day). I've fixed a few problems with
>>> the tests under sqlite and they pass now, but there are still problems with
>>> tests in postgres.
>>> Seb changed lots of 'rebuild_db' statements to 'clean_db', which then
>>> requires 'init_db' at the start of the next test. So there are now 'init_db'
>>> statements at the start of a great many tests. Was there any reason for
>>> this?

Yes, see below.  My intent was for them to be at the start of *all*
tests.  Clearly for whatever reason the fact they weren't was being
hidden by another bug.

>> This may not be necessary any more with the fix we did the other day on
>> migrate.  However, the in memory sqlite database gets lost mysteriously
>> sometimes and its hard to figure out where.  That is why I thought the
>> init_dbs were there.
>
> These problems are with postgres in this case, and there is no mystery about
> losing the d.b. - we call clean_db!
> Note to Seb: Things were probably working in the past because the
> 'faster_db_hacks' config option wasn't being read properly, which David and
> I fixed last Friday, so the full init_db was happening even with postgres,
> whereas it's not now.

Ah, I wondered about that, seeing as the tests all passed before.
Apologies -- at least now I know about the asbool converter :)  As
below, my intent was always that the database *should* be recreated
with each test.

> Also the init_db parameter 'conditional' (BTW what a
> terrible name for a parameter...!) is not being used.

It's not the best, no, but let's not start picking on each others'
individual variable name errors :)  It's an accidental artefact of an
earlier refactoring, just get rid of it...

> I think basically this
> all needs a bit of a review / tidy up to get it to make sense and work too.

Either there's something I don't fundamentally get, or we are talking
cross purposes.  Let me try to untangle at least my own intent:

>>> It seems a right faff to remember this - is there no better way? Indeed I
>>> believe the current test failures are caused by a test not having a init_db.

When refactoring the tests, I noticed that some called "clean_db" and
"init_db" and others didn't.  This was inconsistent.  Most of the
tests assumed test fixtures had been set up, but some had to assume
they'd been set up but not reset.  This state of affairs is much
*more* of a faff when trying to debug problems, than knowing that each
test is responsible for setting up the fixtures it needs to run.
Previously, some (but not all) of the tests had a dependency on some
of the others.

For example there were loads of tests that called "rebuild_db()" in
their teardown methods (which just called "clean_db" and then
"init_db").  More explicit (and standalone) for them to call init_db
in their setup and clean_db in their teardown.  Less faff when trying
to debug problems.

 https://bitbucket.org/okfn/ckan/changeset/e60cf442ad5b#chg-ckan/tests/functional/test_authorization_group.py_oldline27

For me, a principle of good unit testing is that it should be possible
to run each test independently of the others.  The previous setup
required you to run all the tests at once (or a hard-to-discover
subset -- some tests ran stand-alone, others didn't).

So, instead of setting up one test fixture for all the tests, and then
requiring each test to reset the data (sometimes by deleting
individual rows, sometimes by recreating the entire fixture set), it
felt much more explicit to move to the pattern of every test being
responsible for setting up the data it needs.  (Ideally this would
mean we'd actually factor out many of the bits of init_db so that
individual tests wouldn't have to load the whole lot)

That's the thinking!

>>> Note for everyone: Seb, James and I have agreed to set in test.ini to use
>>> sqlite.
>>
>> I disagree to this.  You should specify what database you want in your own
>> development.ini.  The template development.ini could have sqlite as the
>> default instead.  This is the only sane way to do it in my opinion.
>
> Yes this might be better. Anyone else?

Ideally I'd go for neither, and just expect people to read the
documentation, and chose to run tests against the DB of their choice
as they desire.  Really, it's not sane either to develop against a
database we never use in production; sqlite was always a hack to
encourage people to run the tests *at all*.

However, if we are optimising for
making-sure-at-least-a-subset-of-the-tests-are-being-run-at-all,
putting it in test.ini is preferable (if not entirely "sane";),
because people never read the documentation, and getting the tests to
run fast in postgres does require people to read the documentation.

>>> We think it is a good idea to default tests to being run quickly.
>>> However, I can't see a good way to then run tests under postgres - if you
>>> comment out the sqlite in test.ini (as the buildbot currently does), 9/10
>>> times you end up checking it in by mistake. Maybe someone has a better idea,
>>> since passing parameters to nose is a no no.
>>
>> Why is this a no no?
>
> From what I read, nose doesn't believe in passing parameters onto tests -
> they should be stateless. You can fudge it by hacking argv, but then you
> lose flexibility of the many useful nose command-line params.

Have a separate buildbot.ini?

Seb