[ckan-dev] Testing: a helper?

Wed Mar 6 18:23:47 UTC 2013

Hey Sean,

2013/3/6 Sean Hammond <sean.hammond at okfn.org>

> I completely disagree with both of you! This might be a good thing to
> discuss at our dev meeting on Thursday..
>

:P

> I don't understand why these helper methods would be used only for
> setup and not for actually testing.

The thing is that simply needing a user (or organization, whatever) created
for a test is much more common than trying to test that the user creation
actually works. So, while I think that a create_user() is useful for many
tests, I would say that create_user_via_api() is only useful if we're
actually testing the user creation via the api (and, as such, I would
rather put that logic in the test, and not in a helper).

But, obviously, the helper itself needs tests, so we guarantee that it's
create_user(), whatever it's using (if the API or the code), works.

> In ckan you can create
> users via the api, or by the web interface, so we need functional
> tests for both ways, so we need create_user_via_web() and
> create_user_via_api(). Admittedly create_user_via_api() would just by
> a trivial wrapper around call_action_api() so maybe it's not even
> needed, but I think it might make things more readable.
>

Again, these tests are important, but only when you're actually testing
that functionality, and not in every random test that needs an user.

...
>
> I hear what you're saying: if we are just creating a user as part of
> the setup for our test then we don't care about whether it's created
> via the api or web interface. But for testing the create user
> functionality itself, we're going to need create_user_via_api() and
> create_user_via_web() anyway, so why not just reuse them for setup?
> Why make an additional create_user() method? We could name them better
> though e.g. it could be two helper modules like api.create_user() and
> web.create_user()
>

Because that adds noise to the test code. I agree that explicit is better
than implicit, but that's not the case all the time. We need abstractions,
so we focus on what's important. If we follow your line of thought, we
would end creating something like create_user_via_api_v1().

I understand that, if we just have create_user(), we would have to repeat
most of its code whenever we actually need to test the user creation via
the API. Although I would like to keep the tests DRY, I'm OK with this
repetition.

If we really need to reset ckan after each test module (and recreating
> the wsgi and test apps doesn't do it) then that's a pain, but I'd make
> a ticket to fix that in ckan, and in the meantime do it in each test
> module, rather than using a base class to do it by magic. It seems to
> be that in trying to write tests for CKAN we've discovered something
> awkward about using CKAN (the need to reset stuff), and we should fix
> that in CKAN rather than papering-over it with test cleverness that
> makes it look (from the point of view of a test module) like you don't
> have to reset CKAN when in fact you do.
>

Totally agree with you in the case of resetting the app.

But I guess some of that setUp/teardown code will be really hard to get rid
of, mainly the _original_config. We need to keep the tests as independent
as possible from each other, so we need to clear up anything that we
create, be it into the database (using and rolling back from transactions),
or be it config changes (by saving the original and rolling back into the
teardown). You could argue that each test that changes the config is
responsible of clearing it up afterwards, but I guess this pattern is
common enough to justify putting it into a common (helper?) class.

Cheers!
Vítor.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20130306/e502816a/attachment-0001.html>