ckan/ckan — pull request #1736 by seanh (2014-06-02 18:36:00+00:00)
Title: [wip] Test factories should not use the site user

I'm writing a simple function to return the IDs of all a site's editors and admins:

```
def editors_and_admins():
'''Return the IDs of all group or organization editors and admins.

Return a set of the user IDs of all users who are editors or admins of one
or more groups or organizations.

'''
import ckan.model
query = ckan.model.Session.query(ckan.model.Member)
query = query.filter_by(table_name='user')
query = query.filter(ckan.model.Member.capacity.in_(('editor', 'admin')))
return set([member.table_id for member in query.all()])
```

Here's a simple unit test for the function:

```
def test_editors_and_admins_with_1_duplicate_editor_and_admin():

helpers.reset_db()
group = factories.Group()
user = factories.User()
helpers.call_action('group_member_create', id=group['id'],
username=user['name'], role='editor')

assert plugin.editors_and_admins() == set([user['id']])
```

There's nothing wrong with the function, but the test fails because `editors_and_admins()` returns two different user IDs, instead of the one user ID as expected.

The reason is that in CKAN, a user is required to create a group or organization because that user will become the first admin of the group or org. The factories hide this, they let you create a group or org without passing any arguments, and they silently insert the site user into the context. So now, `editors_and_admins()` returns two user IDs: the user who I explicitedly added to the group as an editor, and the site user who was inserted as the group's first admin.

Avoiding this kind of "cleverness" hidden in test helper functions or base classes in different files from the failing test function itself, was one of our main goals when designing the new testing guidelines. We wanted to make things explicit instead, so that each test function was trivial to understand in isolation without having to look up helpers in other files.

Even just the existence of `new_tests/helpers.py` and `new_tests/factories.py` were reluctant compromises.

I think inserting the site user into the context is confusing and breaks the clarity guideline from the testing guidelines. Imho from reading the test code it looks like I created a group with no members, then added one member to it. In fact I created a group with one member (the site user), then added a second member to it, We should make the first member explicit in the test code, by making the factory require it as a param:

```
def test_editors_and_admins_with_1_duplicate_editor_and_admin():

helpers.reset_db()
user = factories.User()
group = factories.Group(user=user)

assert plugin.editors_and_admins() == set([user['id']])
```

Also, creating groups and orgs with the site user is weird - that would never happen in normal usage of the CKAN API or web interface, so it probably shouldn't be what we do in the tests either.