[#1117] Lots of work on the testing coding standards

CONTRIBUTING.rst

@@ -59,12 +59,14 @@ When writing code for CKAN, try to respect our coding standards:
    html-coding-standards
    css-coding-standards
    javascript-coding-standards
-
-* `CKAN Coding Standards <http://docs.ckan.org/en/latest/ckan-coding-standards.html>`_
-* `Python Coding Standards <http://docs.ckan.org/en/latest/python-coding-standards.html>`_
-* `HTML Coding Standards <http://docs.ckan.org/en/latest/html-coding-standards.html>`_
-* `CSS Coding Standards <http://docs.ckan.org/en/latest/css-coding-standards.html>`_
-* `JavaScript Coding Standards <http://docs.ckan.org/en/latest/javascript-coding-standards.html>`_
+   testing-coding-standards
+
+* `CKAN coding standards <http://docs.ckan.org/en/latest/ckan-coding-standards.html>`_
+* `Python coding standards <http://docs.ckan.org/en/latest/python-coding-standards.html>`_
+* `HTML coding standards <http://docs.ckan.org/en/latest/html-coding-standards.html>`_
+* `CSS coding standards <http://docs.ckan.org/en/latest/css-coding-standards.html>`_
+* `JavaScript coding standards <http://docs.ckan.org/en/latest/javascript-coding-standards.html>`_
+* `Testing coding standards <http://docs.ckan.org/en/latest/testing-coding-standards.html>`_
 
 
 ---------------

ckan/new_tests/controllers/__init__.py

@@ -0,0 +1,52 @@
+'''
+
+.. todo::
+
+   Write the tests for one controller, figuring out the best way to write
+   controller tests. Then fill in this guidelines section, using the first set
+   of controller tests as an example.
+
+   Some things have been decided already:
+
+   * All controller methods should have tests
+
+   * Controller tests should be high-level tests that work by posting simulated
+     HTTP requests to CKAN URLs and testing the response. So the controller
+     tests are also testing CKAN's templates and rendering - these are CKAN's
+     front-end tests.
+
+     For example, maybe we use a webtests testapp and then use beautiful soup
+     to parse the HTML?
+
+   * In general the tests for a controller shouldn't need to be too detailed,
+     because there shouldn't be a lot of complicated logic and code in
+     controller classes. The logic should be handled in other places such as
+     :mod:`ckan.logic` and :mod:`ckan.lib`, where it can be tested easily and
+     also shared with other code.
+
+   * The tests for a controller should:
+
+     * Make sure that the template renders without crashing.
+
+     * Test that the page contents seem basically correct, or test certain
+       important elements in the page contents (but don't do too much HTML
+       parsing).
+
+     * Test that submitting any forms on the page works without crashing and
+       has the expected side-effects.
+
+     * When asserting side-effects after submitting a form, controller tests
+       should user the :func:`ckan.new_tests.helpers.call_action` function. For
+       example after creating a new user by submitting the new user form, a
+       test could call the :func:`~ckan.logic.action.get.user_show` action
+       function to verify that the user was created with the correct values.
+
+.. warning::
+
+   Some CKAN controllers *do* contain a lot of complicated logic code.  These
+   controllers should be refactored to move the logic into :mod:`ckan.logic` or
+   :mod:`ckan.lib` where it can be tested easily.  Unfortunately in cases like
+   this it may be necessary to write a lot of controller tests to get this
+   code's behavior into a test harness before it can be safely refactored.
+
+'''

ckan/new_tests/factories.py

@@ -1,37 +1,39 @@
-'''A collection of factory classes for building CKAN users, datasets, etc.
+'''This is a collection of factory classes for building CKAN users, datasets,
+etc.
 
-These are meant to be used by tests to create any objects or "test fixtures"
-that are needed for the tests. They're written using factory_boy:
+These are meant to be used by tests to create any objects that are needed for
+the tests. They're written using ``factory_boy``:
 
-    http://factoryboy.readthedocs.org/en/latest/
+http://factoryboy.readthedocs.org/en/latest/
 
 These are not meant to be used for the actual testing, e.g. if you're writing a
-test for the user_create action function then call user_create, don't test it
-via the User factory below.
+test for the :py:func:`~ckan.logic.action.create.user_create` function then
+call :py:func:`~ckan.new_tests.helpers.call_action`, don't test it
+via the :py:class:`~ckan.new_tests.factories.User` factory below.
 
-Usage:
+Usage::
 
-    # Create a user with the factory's default attributes, and get back a
-    # user dict:
-    user_dict = factories.User()
+ # Create a user with the factory's default attributes, and get back a
+ # user dict:
+ user_dict = factories.User()
 
-    # You can create a second user the same way. For attributes that can't be
-    # the same (e.g. you can't have two users with the same name) a new value
-    # will be generated each time you use the factory:
-    another_user_dict = factories.User()
+ # You can create a second user the same way. For attributes that can't be
+ # the same (e.g. you can't have two users with the same name) a new value
+ # will be generated each time you use the factory:
+ another_user_dict = factories.User()
 
-    # Create a user and specify your own user name and email (this works
-    # with any params that CKAN's user_create() accepts):
-    custom_user_dict = factories.User(name='bob', email='bob@bob.com')
+ # Create a user and specify your own user name and email (this works
+ # with any params that CKAN's user_create() accepts):
+ custom_user_dict = factories.User(name='bob', email='bob@bob.com')
 
-    # Get a user dict containing the attributes (name, email, password, etc.)
-    # that the factory would use to create a user, but without actually
-    # creating the user in CKAN:
-    user_attributes_dict = factories.User.attributes()
+ # Get a user dict containing the attributes (name, email, password, etc.)
+ # that the factory would use to create a user, but without actually
+ # creating the user in CKAN:
+ user_attributes_dict = factories.User.attributes()
 
-    # If you later want to create a user using these attributes, just pass them
-    # to the factory:
-    user = factories.User(**user_attributes_dict)
+ # If you later want to create a user using these attributes, just pass them
+ # to the factory:
+ user = factories.User(**user_attributes_dict)
 
 '''
 import factory
@@ -41,7 +43,7 @@
 import ckan.new_tests.helpers as helpers
 
 
-def generate_email(user):
+def _generate_email(user):
     '''Return an email address for the given User factory stub object.'''
 
     return '{0}@ckan.org'.format(user.name).lower()
@@ -64,7 +66,7 @@ class User(factory.Factory):
 
     # Compute the email param for each user based on the values of the other
     # params above.
-    email = factory.LazyAttribute(generate_email)
+    email = factory.LazyAttribute(_generate_email)
 
     # I'm not sure how to support factory_boy's .build() feature in CKAN,
     # so I've disabled it here.

ckan/new_tests/helpers.py

@@ -1,4 +1,20 @@
-'''A collection of test helper functions.
+'''This is a collection of helper functions for use in tests.
+
+We want to avoid using sharing test helper functions between test modules as
+much as possible, and we definitely don't want to share test fixtures between
+test modules, or to introduce a complex hierarchy of test class subclasses,
+etc.
+
+We want to reduce the amount of "travel" that a reader needs to undertake to
+understand a test method -- reducing the number of other files they need to go
+and read to understand what the test code does. And we want to avoid tightly
+coupling test modules to each other by having them share code.
+
+But some test helper functions just increase the readability of tests so much
+and make writing tests so much easier, that it's worth having them despite the
+potential drawbacks.
+
+This module is reserved for these very useful functions.
 
 '''
 import ckan.model as model
@@ -8,13 +24,15 @@
 def reset_db():
     '''Reset CKAN's database.
 
-    If a test class uses the database, then it should call this function in
-    its setup() method to make sure that it has a clean database to start with
+    If a test class uses the database, then it should call this function in its
+    ``setup()`` method to make sure that it has a clean database to start with
     (nothing left over from other test classes or from previous test runs).
 
     If a test class doesn't use the database (and most test classes shouldn't
     need to) then it doesn't need to call this function.
 
+    :returns: ``None``
+
     '''
     # Close any database connections that have been left open.
     # This prevents CKAN from hanging waiting for some unclosed connection.
@@ -28,21 +46,34 @@ def reset_db():
 
 
 def call_action(action_name, context=None, **kwargs):
-    '''Call the given ckan.logic.action function with the given context
-    and params.
+    '''Call the named ``ckan.logic.action`` and return the result.
 
-    For example:
+    For example::
 
         call_action('user_create', name='seanh', email='seanh@seanh.com',
                     password='pass')
 
+    Any keyword arguments given will be wrapped in a dict and passed to the
+    action function as its ``data_dict`` argument.
+
     This is just a nicer way for user code to call action functions, nicer than
-    either calling the action function directly or via get_action().
+    either calling the action function directly or via
+    :py:func:`ckan.logic.get_action`.
 
-    If accepted this function should eventually be moved to
-    ckan.logic.call_action() and the current get_action() function should be
+    This function should eventually be moved to
+    :py:func:`ckan.logic.call_action` and the current
+    :py:func:`ckan.logic.get_action` function should be
     deprecated. The tests may still need their own wrapper function for
-    logic.call_action(), e.g. to insert 'ignore_auth': True into the context.
+    :py:func:`ckan.logic.call_action`, e.g. to insert ``'ignore_auth': True``
+    into the ``context`` dict.
+
+    :param action_name: the name of the action function to call, e.g.
+        ``'user_update'``
+    :type action_name: string
+    :param context: the context dict to pass to the action function
+        (optional, if no context is given a default one will be supplied)
+    :type context: dict
+    :returns: the dict or other value that the action function returns
 
     '''
     if context is None:
@@ -53,31 +84,30 @@ def call_action(action_name, context=None, **kwargs):
 
 
 def call_auth(auth_name, context, **kwargs):
-    '''Call a ckan.logic.auth function and return the result.
+    '''Call the named ``ckan.logic.auth`` function and return the result.
 
     This is just a convenience function for tests in
-    ckan.new_tests.logic.auth to use.
+    :py:mod:`ckan.new_tests.logic.auth` to use.
 
-    Usage:
+    Usage::
 
-        result = self._call_auth('user_update', context=context,
-                                    id='some_user_id',
-                                    name='updated_user_name')
+        result = helpers.call_auth('user_update', context=context,
+                                   id='some_user_id',
+                                   name='updated_user_name')
 
     :param auth_name: the name of the auth function to call, e.g.
         ``'user_update'``
     :type auth_name: string
 
     :param context: the context dict to pass to the auth function, must
-        contain 'user' and 'model' keys,
+        contain ``'user'`` and ``'model'`` keys,
         e.g. ``{'user': 'fred', 'model': my_mock_model_object}``
     :type context: dict
 
-    :param kwargs: any arguments to be passed to the auth function, these
-        will be wrapped in a dict and passed to the auth function as its
-        ``data_dict`` argument
-
-    :type kwargs: keyword arguments
+    :returns: the dict that the auth function returns, e.g.
+        ``{'success': True}`` or ``{'success': False, msg: '...'}``
+        or just ``{'success': False}``
+    :rtype: dict
 
     '''
     import ckan.logic.auth.update

ckan/new_tests/lib/__init__.py

@@ -0,0 +1,17 @@
+'''**All lib functions should have tests**.
+
+.. todo::
+
+   Write the tests for one ``ckan.lib`` module, figuring out the best way
+   to write lib tests. Then fill in this guidelines section, using the first
+
+
+   We probably want to make these unit tests rather than high-level tests and
+   mock out ``ckan.model``, so the tests are really fast and simple.
+
+   Note that some things in lib are particularly important, e.g. the functions
+   in :py:mod:`ckan.lib.helpers` are exported for templates (including
+   extensions) to use, so all of these functions should really have tests and
+   docstrings. It's probably worth focusing on these modules first.
+
+'''

ckan/new_tests/logic/action/__init__.py

@@ -0,0 +1,35 @@
+'''**All action functions should have tests.**
+
+Most action function tests will be high-level tests that both test the code in
+the action function itself, and also indirectly test the code in
+:mod:`ckan.lib`, :mod:`ckan.model`, :mod:`ckan.logic.schema` etc. that the
+action function calls.
+
+Tests for action functions should use the
+:func:`ckan.new_tests.helpers.call_action` function to call the action
+functions.
+
+One thing :func:`~ckan.new_tests.helpers.call_action` does is to add
+``ignore_auth: True`` into the ``context`` dict that's passed to the action
+function, so that CKAN will not call the action function's authorization
+function.  The tests for an action function *don't* need to cover
+authorization, because the authorization functions have their own tests in
+:mod:`ckan.new_tests.logic.auth`. But action function tests *do* need to cover
+validation, more on that later.
+
+Action function tests *should* test the logic of the actions themselves, and
+*should* test validation (e.g. that various kinds of valid input work as
+expected, and invalid inputs raise the expected exceptions).
+
+Here's an example of a simple :mod:`ckan.logic.action` test:
+
+.. literalinclude:: ../ckan/new_tests/logic/action/test_update.py
+   :start-after: ## START-AFTER
+   :end-before: ## END-BEFORE
+
+.. todo::
+
+   Insert the names of all tests for ``ckan.logic.action.update.user_update``,
+   for example, to show what level of detail things should be tested in.
+
+'''