okfn/ckan — pull request #1034 by seanh (2013-06-21 16:04:20+00:00)
Title: [discuss] Organization purge tests with ckanapi client

@tobes @wardi

For discussion, this is a version of pull request (group and organization purging) with the organization_purge functional tests done using [ckanapi](https://github.com/open-data/ckanapi) instead.

This is what I'd suggest instead of test helper methods, at least for functional tests that use the API. This is for the API only, it doesn't cover using the web frontend, where you need to do things like login or logout of CKAN in code, for that I think you probably do want some test helper methods.

The point of test helper methods for setting up test fixtures, would be to make doing things like creating packages, users, organizations, etc. into simple Pythonic one-liners, so that the tests are concise and readable. But the point of a Python API client is to do the exact same thing. So my argument is that we should make @wardi's API client our official API client, and the tests should just use the API client. That way wer'e not inventing the same wheel twice, and we're dogfooding our own API and API client when writing our tests.

As a side benefit, we would get a good API client that we know is up to date and working (because the CKAN core tests use it) and we can recommend to others. Since we talk about CKAN's amazing open data API so much, it's a bit shameful that we haven't had an API client that we would recommend (or use ourselves) for a long time.

Some notes on how these tests turned out:

I'm actually using a modified ckanapi, I made some tweaks to make it more convenient for these tests, I'll send some pull requests to ckanapi

I don't like how I have to do ``self.api.action.`` every time, that's too long:

* Can ckanapi be changed to enable e.g. ``self.api.user_list()``?

* Or maybe test classes like ``TestOrganizationPurging`` should inherit from
``ckanapi.TestAppCKAN``, then they could do ``self.action.user_list()``?

I'm passing ``apikey=self.sysadmin.apikey`` every time because I like it to be explicit when I'm calling an action as a sysadmin, but I could have just initialized ``self.api`` with the apikey in the first place, then I wouldn't have to pass it each time.

Wherever things start to get awkward in these tests, I think the CKAN API is to blame and should be fixed, rather than papering over the awkwardness with helper functions to make it nicer (meanwhile continuing to publish an API that's a PITA to use). Examples:

There's no good way to create a sysadmin user in CKAN (have to access ``ckan.model`` directly). OK, maybe you wouldn't want to do this over the API. But CKAN should provide a Right Way for tests and paster commands to create a sysadmin and this should be part of CKAN not a test helper.

When creating a group or organization, you can't pass a convenient list of package names or user names, you have to pass these lists of dicts that are a pain to construct:

```
organization = self.api.action.organization_create(
apikey=self.sysadmin.apikey,
name=organization_name,
users=[
{'name': self.org_member['name'],
'capacity': 'member',
},
{'name': self.org_editor['name'],
'capacity': 'editor',
},
{'name': self.org_admin['name'],
'capacity': 'admin',
}]
)
```

(For users the dicts take key "name", but for packages they take key "id".)

This sort of thing also happens when doing e.g. ``organization_show()``, instead of a convenient list of package names and a list of user names, you get lists of dicts, and the result is that the assert lines in the tests get really long and awkward.

The exceptions raised by ``ckan.logic`` seems to be inconsistent in terms of what properties the exception has: a dict as ``.extra_msg``, or a string as ``.extra_msg``, or a dict as ``.error_dict``, etc. This should be made consistent within CKAN, otherwise the tests and anyone using the CKAN API is gonna need their own wrapper code to make it usable.

* * *

Other general stuff about these tests:

I've tried to follow the [Pylons Unit Testing Guidelines](http://docs.pylonsproject.org/en/latest/community/testing.html), for example that each test method tests one thing only, has a method name that describes the condition it tests, and follows the same simple form: do any setup you need; call the method under test _once_; do assertions to make sure it worked; do nothing more. Within reason, allow repetitive code in the tests rather than introduce complexity.

Even though the Pylons Unit Testing Guidelines recommend against it, I did use ``setup_class()`` to setup some test fixture stuff. I think this is okay for *read-only* fixture data, as long as none of the tests are going to modify this stuff, there's no point in setting it up again for each test method.

``_organization_create()`` is a test fixture. Reason for having this in a shared helper method within the module, instead of each test method just doing the work itself, is that it's not just one call to the CKAN API, it's a few calls and then some asserts to make sure it worked (because I don't trust the orgs and groups APIs, I don't think they have a lot of tests). I think shared test fixtures like this belong _within test modules_, not shared between them. Even within a test module, too much of this stuff and the tests become a problem. If you're not convinced, just try to understand [test_activity.py](https://github.com/okfn/ckan/blob/master/ckan/tests/functional/api/test_activity.py) or [test_follow.py](thttps://github.com/okfn/ckan/blob/master/ckan/tests/functional/api/test_activity.py)

I deliberately didn't use ``setup()`` (the one that's called once for each method) for the stuff that I put in ``_organization_create()`` because there might be some test methods in the class that don't need it.

I wanted to test that you can successfully purge an org by passing its name _or_ by passing its id, but that would have meant two test methods that were exactly the same (including the setup and lots of asserts to make sure the purge really happened). So I made them both call a ``_test_organization_purge()`` helper class that does the actual testing. I think this is OK, but again too much of this (even within a test module) and it becomes a problem, this example is about as complex as I'd like it to get.