[ckan-dev] Testing: a helper?

[Vitor Baptista]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20130305/09247a93/attachment-0001.html>