[#1117] Discourage overuse of mocking

Tweak the testing guidelines to discourage overuse of mocking.

ckan/new_tests/controllers/__init__.py

@@ -1,4 +1,5 @@
 '''
+Controller tests probably shouldn't use mocking.
 
 .. todo::
 

ckan/new_tests/logic/action/__init__.py

@@ -3,7 +3,8 @@
 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.
+action function calls. This means that most action function tests should *not*
+use mocking.
 
 Tests for action functions should use the
 :func:`ckan.new_tests.helpers.call_action` function to call the action

ckan/new_tests/logic/action/test_update.py

@@ -344,75 +344,3 @@ def test_user_update_does_not_return_reset_key(self):
 
         updated_user = helpers.call_action('user_update', **params)
         assert 'reset_key' not in updated_user
-
-    ## START-COMPLEX-MOCK-EXAMPLE
-
-    def test_user_update_with_deferred_commit(self):
-        '''Test that user_update()'s deferred_commit option works.
-
-        In this test we mock out the rest of CKAN and test the user_update()
-        action function in isolation. What we're testing is simply that when
-        called with 'deferred_commit': True in its context, user_update() does
-        not call ckan.model.repo.commit().
-
-        '''
-        # Patch ckan.model, so user_update will be accessing a mock object
-        # instead of the real model.
-        # It's ckan.logic.__init__.py:get_action() that actually adds the model
-        # into the context dict, and that module does
-        # `import ckan.model as model`, so what we actually need to patch is
-        # 'ckan.logic.model' (the name that the model has when get_action()
-        # accesses it), not 'ckan.model'.
-        model_patch = mock.patch('ckan.logic.model')
-        mock_model = model_patch.start()
-
-        # Patch the validate() function, so validate() won't really be called
-        # when user_update() calls it. update.py does
-        # `_validate = ckan.lib.navl.dictization_functions.validate` so we
-        # actually to patch this new name that's assigned to the function, not
-        # its original name.
-        validate_patch = mock.patch('ckan.logic.action.update._validate')
-        mock_validate = validate_patch.start()
-
-        # user_update() is going to call validate() with some params and it's
-        # going to use the result that validate() returns. So we need to give
-        # a mock validate function that will accept the right number of params
-        # and return the right result.
-        def mock_validate_function(data_dict, schema, context):
-            '''Simply return the data_dict as given (without doing any
-            validation) and an empty error dict.'''
-            return data_dict, {}
-        mock_validate.side_effect = mock_validate_function
-
-        # Patch model_save, again update.py does
-        # `import ckan.logic.action.update.model_save as model_save` so we
-        # need to patch it by its new name
-        # 'ckan.logic.action.update.model_save'.
-        model_save_patch = mock.patch('ckan.logic.action.update.model_save')
-        model_save_patch.start()
-
-        # Patch model_dictize, again using the new name that update.py imports
-        # it as.
-        model_dictize_patch = mock.patch(
-            'ckan.logic.action.update.model_dictize')
-        model_dictize_patch.start()
-
-        # Patch the get_action() function (using the name that update.py
-        # assigns to it) with a default mock function that does nothing and
-        # returns None.
-        get_action_patch = mock.patch('ckan.logic.action.update._get_action')
-        get_action_patch.start()
-
-        # After all that patching, we can finally call user_update passing
-        # 'defer_commit': True. The logic code in user_update will be run but
-        # it'll have no effect because everything it calls is mocked out.
-        helpers.call_action('user_update',
-                            context={'defer_commit': True},
-                            id='foobar', name='new_name',
-                            )
-
-        # Assert that user_update did *not* call our mock model object's
-        # model.repo.commit() method.
-        assert not mock_model.repo.commit.called
-
-    ## END-COMPLEX-MOCK-EXAMPLE

doc/testing-coding-standards.rst

@@ -41,9 +41,6 @@ Guidelines for writing new-style tests
 We want the tests in :mod:`ckan.new_tests` to be:
 
 Fast
-  * Use the ``mock`` library to avoid pulling in other parts of CKAN
-    (especially the database), see :ref:`mock`.
-
   * Don't share setup code between tests (e.g. in test class ``setup()`` or
     ``setup_class()`` methods, saved against the ``self`` attribute of test
     classes, or in test helper modules).
@@ -52,6 +49,9 @@ Fast
     and have each test method call just the helpers it needs to do the setup
     that it needs.
 
+  * Where appropriate, use the ``mock`` library to avoid pulling in other parts
+    of CKAN (especially the database), see :ref:`mock`.
+
 Independent
   * Each test module, class and method should be able to be run on its own.
 
@@ -269,6 +269,30 @@ faster (especially when :py:mod:`ckan.model` is mocked out so that the tests
 don't touch the database). With mock objects we can also make assertions about
 what methods the function called on the mock object and with which arguments.
 
+.. note::
+
+   Overuse of mocking is discouraged as it can make tests difficult to
+   understand and maintain. Mocking can be useful and make tests both faster
+   and simpler when used appropriately. Some rules of thumb:
+
+   * Don't mock out more than one or two objects in a single test method.
+
+   * Don't use mocking in more functional-style tests. For example the action
+     function tests in :py:mod:`ckan.new_tests.logic.action` and the
+     frontend tests in :py:mod:`ckan.new_tests.controllers` are functional
+     tests, and probably shouldn't do any mocking.
+
+   * Do use mocking in more unit-style tests. For example the authorization
+     function tests in :py:mod:`ckan.new_tests.logic.auth`, the converter and
+     validator tests in :py:mod:`ckan.new_tests.logic.auth`, and most (all?)
+     lib tests in :py:mod:`ckan.new_tests.lib` are unit tests and should use
+     mocking when necessary (often it's possible to unit test a method in
+     isolation from other CKAN code without doing any mocking, which is ideal).
+
+     In these kind of tests we can often mock one or two objects in a simple
+     and easy to understand way, and make the test both simpler and faster.
+
+
 A mock object is a special object that allows user code to access any attribute
 name or call any method name (and pass any parameters) on the object, and the
 code will always get another mock object back:
@@ -301,16 +325,6 @@ out :py:mod:`ckan.model`:
    :start-after: ## START-AFTER
    :end-before: ## END-BEFORE
 
-Here's a much more complex example that patches a number of CKAN modules with
-mock modules, replaces CKAN functions with mock functions with mock behavior,
-and makes assertions about how CKAN called the mock objects (you shouldn't
-have to do mocking as complex as this too often!):
-
-.. literalinclude:: ../ckan/new_tests/logic/action/test_update.py
-   :language: python
-   :start-after: ## START-COMPLEX-MOCK-EXAMPLE
-   :end-before: ## END-COMPLEX-MOCK-EXAMPLE
-
 ----
 
 The following sections will give specific guidelines and examples for writing