[ckan-dev] Testing: a helper?

Wed Mar 6 12:02:05 UTC 2013

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

I don't understand why these helper methods would be used only for
setup and not for actually testing. Surely if I have a
create_organization() helper method for example, then I'd use it in
setup for any tests that needed an organization, but I'd also use it
to test create organization itself, e.g.:

    def test_visitor_cannot_create_org():
        '''Test that visitors are not authorized to create organizations.'''
        api.create_organization(status=403)

    def test_admin_can_create_org_via_api():
        '''Test that sysadmins can create organizations.'''
        api.create_organization(apikey=my_sysadmin.apikey)

etc.

So from that point of view I don't think the fact that it creates the
user via the api is an implementation detail. 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.

I'm not sure how you can lock down the test helper class. It seems
hard to say "we don't want too much clevernes in the tests" and then
start by implementing a subclass containing a bunch of cleverness and
say "but this cleverness is okay." This seems unenforceable over time
as the line is not clearly defined. Whereas no shared code in test
modules apart from this one helper module which contains only
functions and not classes, might be clear enough.

> I agree with Toby. I guess that, for most tests, it doesn't matter if the user is being created using the API or
> directly putting it into the DB, just that it's created correctly. If I read a test that calls "create_user_via_api()", I
> think "does it it only work with users created using the API?"

So if you read a test that calls create_user(), do you think it is
testing create user via both the api and the web frontend? Because if
what create_user() does is use the api then you're kidding yourself,
you haven't tested it by the web frontend, you have a functional test
missing (and when the user registration form starts 500ing you will
not know). Explicit is better than implicit. Of course that argument
only applies if you're using these helper functions to actually test
the functionality and not just for setup, but I don't see why you
wouldn't.

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()

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.

On 5 March 2013 19:06, Vitor Baptista <vitor at vitorbaptista.com> wrote:
> Hey,
>
> I really like this. It'll allow us to remove a LOT of duplication. +1
>
> 2013/3/5 Toby Dacre <toby.okfn at gmail.com>
>>
>> > I think this looks really promising, if we can just build them out of
>> > calls to simple functions like create_user() etc and then asserts, the
>> > functional tests should be really easy to read and write. I think we
>> > should keep the tests themselves pretty high level, so they're almost
>> > like user stories translated directly to unit tests, and almost
>> > human-readable. Then developing a new feature could go: Idea -> User
>> > stories -> Functional tests -> Implementation.
>> >
>> > I think we should follow most of what the Pylons guide says about unit
>> > tests (http://docs.pylonsproject.org/en/latest/community/testing.html)
>> > in our functional tests: each test method only testing one thing, each
>> > method being standalone, good method names that completely describe
>> > the purpose of the test, etc.
>> >
>>
>> It would be good if we could make our tests more readable and simpler
>> to understand and debug when they fail
>
>
> I agree that tests, as all code, should be as readable as possible. But I'm
> not sure of the importance of they being so readable that non-technical
> people could read them. I'm not sure they should/would care, and trying to
> achieve that goal might be more work than it's worth.
>
> I used to love writing tests in Cucumber (http://cukes.info/), that follow
> this to the extreme and actually make the user stories code (python version
> http://urli.st/tUY/jp2). But after a while I thought it was too much work
> for not much added value.
>
>>
>> > Also from the Pylons guide, I think it's important for test modules to
>> > be standalone and not contain too much cleverness or generic shared
>> > code that obscures the tests, and a big part of this is not sharing
>> > code between test modules.
>> >
>> > One thing about functional tests (that's maybe different from unit
>> > tests) is that they tend to do the same thing, e.g. creating a user,
>> > hundreds or thousands of times across all the different functional
>> > tests, most of the times creating the user is not what you're testing
>> > just something you need first in order to get to your test, and each
>> > time that's several lines of code, so having a shared function that
>> > reduces that to one line of code each time is worth it. (The lines of
>> > code saved can be ridiculous:
>> > https://github.com/okfn/ckan/pull/189/files
>> >
>> > But I think we should limit it to a shared helpers module containing
>> > functions, no classes, to reduce the potential for cleverness. In
>> > Toby's example helper class there's already quite a bit of cleverness
>> > creeping in and I think the temptation will mean that this only gets
>> > worse over time.
>
>
> I totally agree that, for tests, code readabiilty is more important than
> reducing code duplication. But there are cases that I'm all in for removing
> duplication.
> I just wouldn't create a cache for the built models, but rely on the
> database. But I understand why Toby did it this way.
>
>>
>> > Some other things:
>> >
>> > I think helper methods need to be named really explicitly, e.g.
>> > create_user_via_api() not create_user().
>> >
>> The reason that this is create_user() not create_user_via_api() is
>> that it is designed to create a user correctly.  Now at the moment it
>> uses the api but that is just an implementation detail.  If I wanted
>> to use the api to create a user (say to test the user api) I would use
>> api_action('create_user', ...).  The idea is that it is a little
>> helper to setup test data correctly.
>
>
> I agree with Toby. I guess that, for most tests, it doesn't matter if the
> user is being created using the API or directly putting it into the DB, just
> that it's created correctly. If I read a test that calls
> "create_user_via_api()", I think "does it it only work with users created
> using the API?"
>
> If the test really needs the user to be created via the API, then yes, I
> agree with the name. If not, it's just noise.
>
>>
>> > They should take params not dicts, I should be able to do
>> > create_user_via_api(name='seanh', email='sean.hammond at okfn.org',
>> > password='xxx'), this is much nicer than having to pass in a dict.
>> >
>>
>> The reason I have done this is so that we do not have name clashes and
>> also partly because how it is done in existing tests.
>> I'm not sure about this it would be a simple change but I fear it
>> might bite us you can use dict(name='...', password='...') for similar
>> effect.
>
>
> I prefer methods that accept params instead of dicts also, because their
> signature tells me what I can pass in (and if, in this case, there're
> optional arguments). If it receives a dict, I have to read its code/docs to
> know this...
>
>>
>> > Toby's create_user() function lets you create a user passing only the
>> > username and it'll generate an email and password for you. I think
>> > this is bad as it obscures what the test code is actually doing. In
>> > CKAN, the fact is that to create a user you have to give a username,
>> > email and password, so the functional tests should reflect this
>> > explicitly.
>> >
>> Again this is about quickly setting up a user for tests - this is NOT
>> a way for testing user api functionality
>> Likewise the other create functions are for setting up test data maybe
>> the names need to be clearer
>
>
> I agree with Toby. My rule of thumb is to be explicit with what you require.
> For example, a test that needs an user into the database could simply call
> create_user(), not caring about its email/name/password. I should only
> bother with these if my test needs them.
>
>> > I don't like the _app, _sys_admin, _config and _original_config in the
>> > helper class, I would let each test class deal with all this itself,
>> > and just provide helper functions so that e.g. making a sysadmin is a
>> > really simple and explicit one-liner.
>> >
>>
>> The reason for _app and others is for the test writer not to have to
>> do too much work.  If I want to just make some api_action() calls why
>> should I have to mess about setting up the app and what if I want to
>> load a test extension or change the config? Why should I need to
>> understand all the intricacies of those systems and risk doing it
>> wrong in my tests.  What if we make changes to how the app gets set-up
>> why should we have to then alter every test?
>>
>> I understand the concern over magic and hidden stuff - I've been
>> bitten enough times by this sort of thing too.  So yes we need to be
>> careful but It feels that if this is all in one place and self
>> documented that at least we can keep them to a minimal.
>
>
> I agree with Toby again. Maybe the configs don't need to be copy()'ed that
> much, but I the knowledge of how to set a flag, or how to reset to the
> original configs, is irrelevant to most (all?) tests. Also, we guarantee
> that every test will clear pylons' config after they're done.
>
> Cheers,
> Vítor.
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
> Unsubscribe: http://lists.okfn.org/mailman/options/ckan-dev
>