[ckan-dev] test failures

Fri Feb 4 11:32:50 UTC 2011

Well the tests are broken - we either go round adding init_db to all the
rest of the test classes (and forever remember to do this for new tests), or
we do a quick sed to do the revert. I'm working now on the latter.

Dave

On 4 February 2011 11:25, James Gardner <james at 3aims.com> wrote:

>  I don't see the case for spending energy reverting anything. I'm fine with
> deleting and recreating the tables with every test class as long as the
> tests aren't slow, which thanks to all the refactoring work they aren't.
>
> James
>
>
>
>
>
> On 04/02/11 11:21, David Read wrote:
>
> 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
>>
>
>
> _______________________________________________
> ckan-dev mailing listckan-dev at lists.okfn.orghttp://lists.okfn.org/mailman/listinfo/ckan-dev
>
>
>
> _______________________________________________
> 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/1bbc39de/attachment-0001.html>