[ckan-dev] test failures

David Read david.read at okfn.org
Fri Feb 4 11:21:26 UTC 2011


On 4 February 2011 10:40, Seb Bacon <seb.bacon at gmail.com> wrote:

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


Ok, but it's used a few times and because the name gives no idea of the
intention it's hard to know whether the init_db calls that use this param
can be removed too. Hence my suggestion for a tidy up.


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


Yes, that's what I'm after - thanks.


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


Looking at cset:c892749f74f0 (before you made changes) there were three
calls to clean_db in the tests. In all occasions they are immediately
followed by a create_db or a rebuild_db. So the tests *were* consistently
expecting the tables to be created.

>

> Most of the
> tests assumed test fixtures had been set up,


I don't believe that is true. All test classes assume the tables are empty.
All over the place you see setup methods creating test data and teardowns
deleting it. When the teardown is lazy, they call rebuild_db, which in the
past was slower, but sure.


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

I agree there shouldn't be dependencies between tests. Certainly there have
been a few test methods which relied on state left by a previous method in
the class. The ones I've come across I've either decided are a reasonable
compromise and labelled very clearly, or I've gone to the trouble to correct
them. Yes there are a few tests which remain 'at large', and we should focus
on stamping them out, rather than throwing the baby out with the bathwater
to delete and recreate the tables with every test class.


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

So you replaced about 60 rebuild_db calls with clean_db and mandated that
every single test starts with init_db. There is no help to debugging by
making sure the tables are dropped in addition to deleting the data. I
maintain that replacing rebuild_db added faff and for the sake of ease of
writing tests and reducing code to maintain we should revert this.

However I think this shows that someone new to our tests needs to be able to
find our test conventions and intentions documented, so that when there are
issues (and it is true there are probably some non-independent tests
lurking) then we can all pull in the same direction to solve them.

David



>
> 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
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20110204/ca9e6ac2/attachment-0001.html>


More information about the ckan-dev mailing list