From 9147540ab8e381a41ab849239b919c19a2641a35 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Tue, 1 Oct 2013 12:17:54 +0200 Subject: [PATCH] [#1117] Discourage overuse of mocking Tweak the testing guidelines to discourage overuse of mocking. --- ckan/new_tests/controllers/__init__.py | 1 + ckan/new_tests/logic/action/__init__.py | 3 +- ckan/new_tests/logic/action/test_update.py | 72 ---------------------- doc/testing-coding-standards.rst | 40 ++++++++---- 4 files changed, 30 insertions(+), 86 deletions(-) diff --git a/ckan/new_tests/controllers/__init__.py b/ckan/new_tests/controllers/__init__.py index 6a901790973..916996429f3 100644 --- a/ckan/new_tests/controllers/__init__.py +++ b/ckan/new_tests/controllers/__init__.py @@ -1,4 +1,5 @@ ''' +Controller tests probably shouldn't use mocking. .. todo:: diff --git a/ckan/new_tests/logic/action/__init__.py b/ckan/new_tests/logic/action/__init__.py index d666170367e..6272c5b24b2 100644 --- a/ckan/new_tests/logic/action/__init__.py +++ b/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 diff --git a/ckan/new_tests/logic/action/test_update.py b/ckan/new_tests/logic/action/test_update.py index dd32c4f7c37..8413a3a8178 100644 --- a/ckan/new_tests/logic/action/test_update.py +++ b/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 diff --git a/doc/testing-coding-standards.rst b/doc/testing-coding-standards.rst index e5082a28de6..2bd89545da2 100644 --- a/doc/testing-coding-standards.rst +++ b/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