diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index cc5dfd67304..7b15f8d5531 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -60,12 +60,14 @@ When writing code for CKAN, try to respect our coding standards: html-coding-standards css-coding-standards javascript-coding-standards - -* `CKAN Coding Standards `_ -* `Python Coding Standards `_ -* `HTML Coding Standards `_ -* `CSS Coding Standards `_ -* `JavaScript Coding Standards `_ + testing-coding-standards + +* `CKAN coding standards `_ +* `Python coding standards `_ +* `HTML coding standards `_ +* `CSS coding standards `_ +* `JavaScript coding standards `_ +* `Testing coding standards `_ --------------- diff --git a/ckan/ckan_nose_plugin.py b/ckan/ckan_nose_plugin.py index bc995dbbdb3..2ec4d0988b9 100644 --- a/ckan/ckan_nose_plugin.py +++ b/ckan/ckan_nose_plugin.py @@ -17,6 +17,10 @@ def startContext(self, ctx): # import needs to be here or setup happens too early import ckan.model as model + if 'new_tests' in repr(ctx): + # We don't want to do the stuff below for new-style tests. + return + if isclass(ctx): if hasattr(ctx, "no_db") and ctx.no_db: return @@ -38,7 +42,7 @@ def startContext(self, ctx): from ckan.plugins.interfaces import IConfigurable for plugin in PluginImplementations(IConfigurable): plugin.configure(config) - + model.repo.init_db() def options(self, parser, env): diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 7796eccb9a0..b3a83235cd6 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -443,6 +443,7 @@ def user_dictize(user, context): result_dict = d.table_dictize(user, context) del result_dict['password'] + del result_dict['reset_key'] result_dict['display_name'] = user.display_name result_dict['email_hash'] = user.email_hash diff --git a/ckan/lib/navl/validators.py b/ckan/lib/navl/validators.py index 01cbd87567f..b7ac124e624 100644 --- a/ckan/lib/navl/validators.py +++ b/ckan/lib/navl/validators.py @@ -76,7 +76,20 @@ def callable(key, data, errors, context): return callable def ignore_missing(key, data, errors, context): + '''If the key is missing from the data, ignore the rest of the key's + schema. + By putting ignore_missing at the start of the schema list for a key, + you can allow users to post a dict without the key and the dict will pass + validation. But if they post a dict that does contain the key, then any + validators after ignore_missing in the key's schema list will be applied. + + :raises ckan.lib.navl.dictization_functions.StopOnError: if ``data[key]`` + is :py:data:`ckan.lib.navl.dictization_functions.missing` or ``None`` + + :returns: ``None`` + + ''' value = data.get(key) if value is missing or value is None: diff --git a/ckan/logic/validators.py b/ckan/logic/validators.py index cccd62af6a8..c4c9b6d3aca 100644 --- a/ckan/logic/validators.py +++ b/ckan/logic/validators.py @@ -281,20 +281,39 @@ def extras_unicode_convert(extras, context): return extras name_match = re.compile('[a-z0-9_\-]*$') -def name_validator(val, context): +def name_validator(value, context): + '''Return the given value if it's a valid name, otherwise raise Invalid. + + If it's a valid name, the given value will be returned unmodified. + + This function applies general validation rules for names of packages, + groups, users, etc. + + Most schemas also have their own custom name validator function to apply + custom validation rules after this function, for example a + ``package_name_validator()`` to check that no package with the given name + already exists. + + :raises ckan.lib.navl.dictization_functions.Invalid: if ``value`` is not + a valid name + + ''' + if not isinstance(value, basestring): + raise Invalid(_('Names must be strings')) + # check basic textual rules - if val in ['new', 'edit', 'search']: + if value in ['new', 'edit', 'search']: raise Invalid(_('That name cannot be used')) - if len(val) < 2: + if len(value) < 2: raise Invalid(_('Name must be at least %s characters long') % 2) - if len(val) > PACKAGE_NAME_MAX_LENGTH: + if len(value) > PACKAGE_NAME_MAX_LENGTH: raise Invalid(_('Name must be a maximum of %i characters long') % \ PACKAGE_NAME_MAX_LENGTH) - if not name_match.match(val): + if not name_match.match(value): raise Invalid(_('Url must be purely lowercase alphanumeric ' '(ascii) characters and these symbols: -_')) - return val + return value def package_name_validator(key, data, errors, context): model = context["model"] @@ -480,20 +499,37 @@ def ignore_not_group_admin(key, data, errors, context): data.pop(key) def user_name_validator(key, data, errors, context): - model = context["model"] - session = context["session"] - user = context.get("user_obj") + '''Validate a new user name. - query = session.query(model.User.name).filter_by(name=data[key]) - if user: - user_id = user.id - else: - user_id = data.get(key[:-1] + ("id",)) - if user_id and user_id is not missing: - query = query.filter(model.User.id <> user_id) - result = query.first() - if result: - errors[key].append(_('That login name is not available.')) + Append an error message to ``errors[key]`` if a user named ``data[key]`` + already exists. Otherwise, do nothing. + + :raises ckan.lib.navl.dictization_functions.Invalid: if ``data[key]`` is + not a string + :rtype: None + + ''' + model = context['model'] + new_user_name = data[key] + + if not isinstance(new_user_name, basestring): + raise Invalid(_('User names must be strings')) + + user = model.User.get(new_user_name) + if user is not None: + # A user with new_user_name already exists in the database. + + user_obj_from_context = context.get('user_obj') + if user_obj_from_context and user_obj_from_context.id == user.id: + # If there's a user_obj in context with the same id as the user + # found in the db, then we must be doing a user_update and not + # updating the user name, so don't return an error. + return + else: + # Otherwise return an error: there's already another user with that + # name, so you can create a new user with that name or update an + # existing user's name to that name. + errors[key].append(_('That login name is not available.')) def user_both_passwords_entered(key, data, errors, context): @@ -507,7 +543,13 @@ def user_both_passwords_entered(key, data, errors, context): def user_password_validator(key, data, errors, context): value = data[key] - if not value == '' and not isinstance(value, Missing) and not len(value) >= 4: + if isinstance(value, Missing): + pass + elif not isinstance(value, basestring): + errors[('password',)].append(_('Passwords must be strings')) + elif value == '': + pass + elif len(value) < 4: errors[('password',)].append(_('Your password must be 4 characters or longer')) def user_passwords_match(key, data, errors, context): diff --git a/ckan/new_tests/__init__.py b/ckan/new_tests/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ckan/new_tests/controllers/__init__.py b/ckan/new_tests/controllers/__init__.py new file mode 100644 index 00000000000..916996429f3 --- /dev/null +++ b/ckan/new_tests/controllers/__init__.py @@ -0,0 +1,53 @@ +''' +Controller tests probably shouldn't use mocking. + +.. 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. + +''' diff --git a/ckan/new_tests/factories.py b/ckan/new_tests/factories.py new file mode 100644 index 00000000000..6e8385eeacf --- /dev/null +++ b/ckan/new_tests/factories.py @@ -0,0 +1,142 @@ +'''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 that are needed for +the tests. They're written using ``factory_boy``: + +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 :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:: + + # 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() + + # 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() + + # 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 +import mock + +import ckan.model +import ckan.logic +import ckan.new_tests.helpers as helpers + + +def _generate_email(user): + '''Return an email address for the given User factory stub object.''' + + return '{0}@ckan.org'.format(user.name).lower() + + +def _generate_reset_key(user): + '''Return a reset key for the given User factory stub object.''' + + return '{0}_reset_key'.format(user.name).lower() + + +def _generate_user_id(user): + '''Return a user id for the given User factory stub object.''' + + return '{0}_user_id'.format(user.name).lower() + + +class User(factory.Factory): + '''A factory class for creating CKAN users.''' + + # This is the class that UserFactory will create and return instances + # of. + FACTORY_FOR = ckan.model.User + + # These are the default params that will be used to create new users. + fullname = 'Mr. Test User' + password = 'pass' + about = 'Just another test user.' + + # Generate a different user name param for each user that gets created. + name = factory.Sequence(lambda n: 'test_user_{n}'.format(n=n)) + + # Compute the email param for each user based on the values of the other + # params above. + 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. + @classmethod + def _build(cls, target_class, *args, **kwargs): + raise NotImplementedError(".build() isn't supported in CKAN") + + # To make factory_boy work with CKAN we override _create() and make it call + # a CKAN action function. + # We might also be able to do this by using factory_boy's direct SQLAlchemy + # support: http://factoryboy.readthedocs.org/en/latest/orms.html#sqlalchemy + @classmethod + def _create(cls, target_class, *args, **kwargs): + if args: + assert False, "Positional args aren't supported, use keyword args." + user_dict = helpers.call_action('user_create', **kwargs) + return user_dict + + +class MockUser(factory.Factory): + '''A factory class for creating mock CKAN users using the mock library.''' + + FACTORY_FOR = mock.MagicMock + + fullname = 'Mr. Mock User' + password = 'pass' + about = 'Just another mock user.' + name = factory.Sequence(lambda n: 'mock_user_{n}'.format(n=n)) + email = factory.LazyAttribute(_generate_email) + reset_key = factory.LazyAttribute(_generate_reset_key) + id = factory.LazyAttribute(_generate_user_id) + + @classmethod + def _build(cls, target_class, *args, **kwargs): + raise NotImplementedError(".build() isn't supported in CKAN") + + @classmethod + def _create(cls, target_class, *args, **kwargs): + if args: + assert False, "Positional args aren't supported, use keyword args." + mock_user = mock.MagicMock() + for name, value in kwargs.items(): + setattr(mock_user, name, value) + return mock_user + + +def validator_data_dict(): + '''Return a data dict with some arbitrary data in it, suitable to be passed + to validator functions for testing. + + ''' + return {('other key',): 'other value'} + + +def validator_errors_dict(): + '''Return an errors dict with some arbitrary errors in it, suitable to be + passed to validator functions for testing. + + ''' + return {('other key',): ['other error']} diff --git a/ckan/new_tests/helpers.py b/ckan/new_tests/helpers.py new file mode 100644 index 00000000000..51640a7e3dd --- /dev/null +++ b/ckan/new_tests/helpers.py @@ -0,0 +1,118 @@ +'''This is a collection of helper functions for use in tests. + +We want to avoid 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 +import ckan.logic as logic + + +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 + (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. + model.Session.close_all() + + model.repo.clean_db() + + +def call_action(action_name, context=None, **kwargs): + '''Call the named ``ckan.logic.action`` function and return the result. + + For example:: + + user_dict = 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 + :py:func:`ckan.logic.get_action`. + + 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 + :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: + context = {} + context.setdefault('user', '127.0.0.1') + context.setdefault('ignore_auth', True) + return logic.get_action(action_name)(context=context, data_dict=kwargs) + + +def call_auth(auth_name, context, **kwargs): + '''Call the named ``ckan.logic.auth`` function and return the result. + + This is just a convenience function for tests in + :py:mod:`ckan.new_tests.logic.auth` to use. + + Usage:: + + 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, + e.g. ``{'user': 'fred', 'model': my_mock_model_object}`` + :type context: dict + + :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 + + assert 'user' in context, ('Test methods must put a user name in the ' + 'context dict') + assert 'model' in context, ('Test methods must put a model in the ' + 'context dict') + + # FIXME: Do we want to go through check_access() here? + auth_function = ckan.logic.auth.update.__getattribute__(auth_name) + return auth_function(context=context, data_dict=kwargs) diff --git a/ckan/new_tests/lib/__init__.py b/ckan/new_tests/lib/__init__.py new file mode 100644 index 00000000000..005e027da2d --- /dev/null +++ b/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. + +''' diff --git a/ckan/new_tests/lib/navl/__init__.py b/ckan/new_tests/lib/navl/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ckan/new_tests/lib/navl/test_validators.py b/ckan/new_tests/lib/navl/test_validators.py new file mode 100644 index 00000000000..dccee431453 --- /dev/null +++ b/ckan/new_tests/lib/navl/test_validators.py @@ -0,0 +1,266 @@ +# -*- coding: utf-8 -*- +'''Unit tests for ckan/lib/navl/validators.py. + +''' +import copy + +import nose.tools + +import ckan.new_tests.factories as factories + + +def returns_None(function): + '''A decorator that asserts that the decorated function returns None. + + :param function: the function to decorate + :type function: function + + Usage: + + @returns_None + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(*args, **kwargs): + original_args = copy.deepcopy(args) + original_kwargs = copy.deepcopy(kwargs) + + result = function(*args, **kwargs) + + assert result is None, ( + 'Should return None when called with args: {args} and ' + 'kwargs: {kwargs}'.format(args=original_args, + kwargs=original_kwargs)) + return result + return call_and_assert + + +def raises_StopOnError(function): + '''A decorator that asserts that the decorated function raises + dictization_functions.StopOnError. + + :param function: the function to decorate + :type function: function + + Usage: + + @raises_StopOnError + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(*args, **kwargs): + import ckan.lib.navl.dictization_functions as df + nose.tools.assert_raises(df.StopOnError, function, *args, **kwargs) + return call_and_assert + + +def does_not_modify_data_dict(validator): + '''A decorator that asserts that the decorated validator doesn't modify + its `data` dict param. + + :param validator: the validator function to decorate + :type validator: function + + Usage: + + @does_not_modify_data_dict + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(key, data, errors, context=None): + if context is None: + context = {} + original_data = copy.deepcopy(data) + original_errors = copy.deepcopy(errors) + original_context = copy.deepcopy(context) + + result = validator(key, data, errors, context=context) + + assert data == original_data, ( + 'Should not modify data dict when called with ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + return result + return call_and_assert + + +def removes_key_from_data_dict(validator): + '''A decorator that asserts that the decorated validator removes its key + from the data dict. + + :param validator: the validator function to decorate + :type validator: function + + Usage: + + @removes_key_from_data_dict + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(key, data, errors, context=None): + if context is None: + context = {} + original_data = copy.deepcopy(data) + original_errors = copy.deepcopy(errors) + original_context = copy.deepcopy(context) + + result = validator(key, data, errors, context=context) + + assert key not in data, ( + 'Should remove key from data dict when called with: ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context} '.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + return result + return call_and_assert + + +def does_not_modify_other_keys_in_data_dict(validator): + '''A decorator that asserts that the decorated validator doesn't add, + modify the value of, or remove any other keys from its ``data`` dict param. + + The function *may* modify its own data dict key. + + :param validator: the validator function to decorate + :type validator: function + + Usage: + + @does_not_modify_other_keys_in_data_dict + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(key, data, errors, context=None): + if context is None: + context = {} + original_data = copy.deepcopy(data) + original_errors = copy.deepcopy(errors) + original_context = copy.deepcopy(context) + + result = validator(key, data, errors, context=context) + + # The validator function is allowed to modify its own key, so remove + # that key from both dicts for the purposes of the assertions below. + if key in data: + del data[key] + if key in original_data: + del original_data[key] + + assert data.keys() == original_data.keys(), ( + 'Should not add or remove keys from data dict when called with ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + for key_ in data: + assert data[key_] == original_data[key_], ( + 'Should not modify other keys in data dict when called with ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + return result + return call_and_assert + + +def does_not_modify_errors_dict(validator): + '''A decorator that asserts that the decorated validator doesn't modify its + `errors` dict param. + + :param validator: the validator function to decorate + :type validator: function + + Usage: + + @does_not_modify_errors_dict + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(key, data, errors, context=None): + if context is None: + context = {} + original_data = copy.deepcopy(data) + original_errors = copy.deepcopy(errors) + original_context = copy.deepcopy(context) + + result = validator(key, data, errors, context=context) + + assert errors == original_errors, ( + 'Should not modify errors dict when called with key: {key}, ' + 'data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + return result + return call_and_assert + + +class TestValidators(object): + + def test_ignore_missing_with_value_missing(self): + '''ignore_missing() should raise StopOnError if: + + - data[key] is None, or + - data[key] is dictization_functions.missing, or + - key is not in data + + ''' + import ckan.lib.navl.dictization_functions as df + import ckan.lib.navl.validators as validators + + for value in (None, df.missing, 'skip'): + + # This is the key for the value that is going to be validated. + key = ('key to be validated',) + + # The data to pass to the validator function for validation. + data = factories.validator_data_dict() + if value != 'skip': + data[key] = value + + # The errors dict to pass to the validator function. + errors = factories.validator_errors_dict() + errors[key] = [] + + @does_not_modify_other_keys_in_data_dict + @does_not_modify_errors_dict + @removes_key_from_data_dict + @raises_StopOnError + def call_validator(*args, **kwargs): + return validators.ignore_missing(*args, **kwargs) + call_validator(key=key, data=data, errors=errors, context={}) + + def test_ignore_missing_with_a_value(self): + '''If data[key] is neither None or missing, ignore_missing() should do + nothing. + + ''' + import ckan.lib.navl.validators as validators + + key = ('key to be validated',) + data = factories.validator_data_dict() + data[key] = 'value to be validated' + errors = factories.validator_errors_dict() + errors[key] = [] + + @returns_None + @does_not_modify_data_dict + @does_not_modify_errors_dict + def call_validator(*args, **kwargs): + return validators.ignore_missing(*args, **kwargs) + call_validator(key=key, data=data, errors=errors, context={}) diff --git a/ckan/new_tests/logic/__init__.py b/ckan/new_tests/logic/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ckan/new_tests/logic/action/__init__.py b/ckan/new_tests/logic/action/__init__.py new file mode 100644 index 00000000000..6272c5b24b2 --- /dev/null +++ b/ckan/new_tests/logic/action/__init__.py @@ -0,0 +1,36 @@ +'''**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. 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 +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. + +''' diff --git a/ckan/new_tests/logic/action/test_update.py b/ckan/new_tests/logic/action/test_update.py new file mode 100644 index 00000000000..8413a3a8178 --- /dev/null +++ b/ckan/new_tests/logic/action/test_update.py @@ -0,0 +1,346 @@ +'''Unit tests for ckan/logic/action/update.py.''' +import datetime + +import nose.tools +import mock + +import ckan.logic as logic +import ckan.new_tests.helpers as helpers +import ckan.new_tests.factories as factories + + +def datetime_from_string(s): + '''Return a standard datetime.datetime object initialised from a string in + the same format used for timestamps in dictized activities (the format + produced by datetime.datetime.isoformat()) + + ''' + return datetime.datetime.strptime(s, '%Y-%m-%dT%H:%M:%S.%f') + + +class TestUpdate(object): + + @classmethod + def setup_class(cls): + + # Initialize the test db (if it isn't already) and clean out any data + # left in it. + # You should only do this in your setup_class() method if your test + # class uses the db, most test classes shouldn't need to. + helpers.reset_db() + + def setup(self): + import ckan.model as model + + # Reset the db before each test method. + # You should only do this in your setup() method if your test class + # uses the db, most test classes shouldn't need to. + model.repo.rebuild_db() + + def teardown(self): + # Since some of the test methods below use the mock module to patch + # things, we use this teardown() method to remove remove all patches. + # (This makes sure the patches always get removed even if the test + # method aborts with an exception or something.) + mock.patch.stopall() + + ## START-AFTER + + def test_user_update_name(self): + '''Test that updating a user's name works successfully.''' + + # The canonical form of a test has four steps: + # 1. Setup any preconditions needed for the test. + # 2. Call the function that's being tested, once only. + # 3. Make assertions about the return value and/or side-effects of + # of the function that's being tested. + # 4. Do nothing else! + + # 1. Setup. + user = factories.User() + + # 2. Call the function that's being tested, once only. + # FIXME we have to pass the email address and password to user_update + # even though we're not updating those fields, otherwise validation + # fails. + helpers.call_action('user_update', id=user['name'], + email=user['email'], + password=factories.User.attributes()['password'], + name='updated', + ) + + # 3. Make assertions about the return value and/or side-effects. + updated_user = helpers.call_action('user_show', id=user['id']) + # Note that we check just the field we were trying to update, not the + # entire dict, only assert what we're actually testing. + assert updated_user['name'] == 'updated' + + # 4. Do nothing else! + + ## END-BEFORE + + def test_user_update_with_id_that_does_not_exist(self): + user_dict = factories.User.attributes() + user_dict['id'] = "there's no user with this id" + + nose.tools.assert_raises(logic.NotFound, helpers.call_action, + 'user_update', **user_dict) + + def test_user_update_with_no_id(self): + user_dict = factories.User.attributes() + assert 'id' not in user_dict + nose.tools.assert_raises(logic.ValidationError, helpers.call_action, + 'user_update', **user_dict) + + ## START-FOR-LOOP-EXAMPLE + + def test_user_update_with_invalid_name(self): + user = factories.User() + + invalid_names = ('', 'a', False, 0, -1, 23, 'new', 'edit', 'search', + 'a'*200, 'Hi!', 'i++%') + for name in invalid_names: + user['name'] = name + nose.tools.assert_raises(logic.ValidationError, + helpers.call_action, 'user_update', + **user) + + ## END-FOR-LOOP-EXAMPLE + + def test_user_update_to_name_that_already_exists(self): + fred = factories.User(name='fred') + bob = factories.User(name='bob') + + # Try to update fred and change his user name to bob, which is already + # bob's user name + fred['name'] = bob['name'] + nose.tools.assert_raises(logic.ValidationError, helpers.call_action, + 'user_update', **fred) + + def test_user_update_password(self): + '''Test that updating a user's password works successfully.''' + + user = factories.User() + + # FIXME we have to pass the email address to user_update even though + # we're not updating it, otherwise validation fails. + helpers.call_action('user_update', id=user['name'], + email=user['email'], + password='new password', + ) + + # user_show() never returns the user's password, so we have to access + # the model directly to test it. + import ckan.model as model + updated_user = model.User.get(user['id']) + assert updated_user.validate_password('new password') + + def test_user_update_with_short_password(self): + user = factories.User() + + user['password'] = 'xxx' # This password is too short. + nose.tools.assert_raises(logic.ValidationError, helpers.call_action, + 'user_update', **user) + + def test_user_update_with_empty_password(self): + '''If an empty password is passed to user_update, nothing should + happen. + + No error (e.g. a validation error) is raised, but the password is not + changed either. + + ''' + user_dict = factories.User.attributes() + original_password = user_dict['password'] + user_dict = factories.User(**user_dict) + + user_dict['password'] = '' + helpers.call_action('user_update', **user_dict) + + import ckan.model as model + updated_user = model.User.get(user_dict['id']) + assert updated_user.validate_password(original_password) + + def test_user_update_with_null_password(self): + user = factories.User() + + user['password'] = None + nose.tools.assert_raises(logic.ValidationError, helpers.call_action, + 'user_update', **user) + + def test_user_update_with_invalid_password(self): + user = factories.User() + + for password in (False, -1, 23, 30.7): + user['password'] = password + nose.tools.assert_raises(logic.ValidationError, + helpers.call_action, 'user_update', + **user) + + def test_user_update_without_email_address(self): + '''You have to pass an email address when you call user_update. + + Even if you don't want to change the user's email address, you still + have to pass their current email address to user_update. + + FIXME: The point of this feature seems to be to prevent people from + removing email addresses from user accounts, but making them post the + current email address every time they post to user update is just + annoying, they should be able to post a dict with no 'email' key to + user_update and it should simply not change the current email. + + ''' + user = factories.User() + del user['email'] + + nose.tools.assert_raises(logic.ValidationError, + helpers.call_action, 'user_update', + **user) + + # TODO: Valid and invalid values for the rest of the user model's fields. + + def test_user_update_activity_stream(self): + '''Test that the right activity is emitted when updating a user.''' + + user = factories.User() + before = datetime.datetime.now() + + # FIXME we have to pass the email address and password to user_update + # even though we're not updating those fields, otherwise validation + # fails. + helpers.call_action('user_update', id=user['name'], + email=user['email'], + password=factories.User.attributes()['password'], + name='updated', + ) + + activity_stream = helpers.call_action('user_activity_list', + id=user['id']) + latest_activity = activity_stream[0] + assert latest_activity['activity_type'] == 'changed user' + assert latest_activity['object_id'] == user['id'] + assert latest_activity['user_id'] == user['id'] + after = datetime.datetime.now() + timestamp = datetime_from_string(latest_activity['timestamp']) + assert timestamp >= before and timestamp <= after + + def test_user_update_with_custom_schema(self): + '''Test that custom schemas passed to user_update do get used. + + user_update allows a custom validation schema to be passed to it in the + context dict. This is just a simple test that if you pass a custom + schema user_update does at least call a custom method that's given in + the custom schema. We assume this means it did use the custom schema + instead of the default one for validation, so user_update's custom + schema feature does work. + + ''' + import ckan.logic.schema + + user = factories.User() + + # A mock validator method, it doesn't do anything but it records what + # params it gets called with and how many times. + mock_validator = mock.MagicMock() + + # Build a custom schema by taking the default schema and adding our + # mock method to its 'id' field. + schema = ckan.logic.schema.default_update_user_schema() + schema['id'].append(mock_validator) + + # Call user_update and pass our custom schema in the context. + # FIXME: We have to pass email and password even though we're not + # trying to update them, or validation fails. + helpers.call_action('user_update', context={'schema': schema}, + id=user['name'], email=user['email'], + password=factories.User.attributes()['password'], + name='updated', + ) + + # Since we passed user['name'] to user_update as the 'id' param, + # our mock validator method should have been called once with + # user['name'] as arg. + mock_validator.assert_called_once_with(user['name']) + + def test_user_update_multiple(self): + '''Test that updating multiple user attributes at once works.''' + + user = factories.User() + + params = { + 'id': user['id'], + 'name': 'updated_name', + 'fullname': 'updated full name', + 'about': 'updated about', + # FIXME: We shouldn't have to put email here since we're not + # updating it, but user_update sucks. + 'email': user['email'], + # FIXME: We shouldn't have to put password here since we're not + # updating it, but user_update sucks. + 'password': factories.User.attributes()['password'], + } + + helpers.call_action('user_update', **params) + + updated_user = helpers.call_action('user_show', id=user['id']) + assert updated_user['name'] == 'updated_name' + assert updated_user['fullname'] == 'updated full name' + assert updated_user['about'] == 'updated about' + + def test_user_update_does_not_return_password(self): + '''The user dict that user_update returns should not include the user's + password.''' + + user = factories.User() + + params = { + 'id': user['id'], + 'name': 'updated_name', + 'fullname': 'updated full name', + 'about': 'updated about', + 'email': user['email'], + 'password': factories.User.attributes()['password'], + } + + updated_user = helpers.call_action('user_update', **params) + assert 'password' not in updated_user + + def test_user_update_does_not_return_apikey(self): + '''The user dict that user_update returns should not include the user's + API key.''' + + user = factories.User() + + params = { + 'id': user['id'], + 'name': 'updated_name', + 'fullname': 'updated full name', + 'about': 'updated about', + 'email': user['email'], + 'password': factories.User.attributes()['password'], + } + + updated_user = helpers.call_action('user_update', **params) + assert 'apikey' not in updated_user + + def test_user_update_does_not_return_reset_key(self): + '''The user dict that user_update returns should not include the user's + reset key.''' + + import ckan.lib.mailer + import ckan.model + + user = factories.User() + ckan.lib.mailer.create_reset_key(ckan.model.User.get(user['id'])) + + params = { + 'id': user['id'], + 'name': 'updated_name', + 'fullname': 'updated full name', + 'about': 'updated about', + 'email': user['email'], + 'password': factories.User.attributes()['password'], + } + + updated_user = helpers.call_action('user_update', **params) + assert 'reset_key' not in updated_user diff --git a/ckan/new_tests/logic/auth/__init__.py b/ckan/new_tests/logic/auth/__init__.py new file mode 100644 index 00000000000..246995c15a6 --- /dev/null +++ b/ckan/new_tests/logic/auth/__init__.py @@ -0,0 +1,17 @@ +'''**All auth functions should have tests.** + +Most auth function tests should be unit tests that test the auth function in +isolation, without bringing in other parts of CKAN or touching the database. +This requires using the ``mock`` library to mock ``ckan.model``, see +:ref:`mock`. + +Tests for auth functions should use the +:func:`ckan.new_tests.helpers.call_auth` function to call auth functions. + +Here's an example of a simple :py:mod:`ckan.logic.auth` test: + +.. literalinclude:: ../ckan/new_tests/logic/auth/test_update.py + :start-after: ## START-AFTER + :end-before: ## END-BEFORE + +''' diff --git a/ckan/new_tests/logic/auth/test_update.py b/ckan/new_tests/logic/auth/test_update.py new file mode 100644 index 00000000000..0970e2ed0a3 --- /dev/null +++ b/ckan/new_tests/logic/auth/test_update.py @@ -0,0 +1,141 @@ +'''Unit tests for ckan/logic/auth.update.py. + +''' +import mock + +import ckan.new_tests.helpers as helpers +import ckan.new_tests.factories as factories + + +class TestUpdate(object): + + def test_user_update_visitor_cannot_update_user(self): + '''Visitors should not be able to update users' accounts.''' + + # Make a mock ckan.model.User object, Fred. + fred = factories.MockUser(name='fred') + + # Make a mock ckan.model object. + mock_model = mock.MagicMock() + # model.User.get(user_id) should return Fred. + mock_model.User.get.return_value = fred + + # Put the mock model in the context. + # This is easier than patching import ckan.model. + context = {'model': mock_model} + + # No user is going to be logged-in. + context['user'] = '127.0.0.1' + + # Make the visitor try to update Fred's user account. + params = { + 'id': fred.id, + 'name': 'updated_user_name', + } + result = helpers.call_auth('user_update', context=context, **params) + + assert result['success'] is False + # FIXME: This is a terrible error message, containing both 127.0.0.1 + # and Fred's user id (not his name). + assert result['msg'] == ('User 127.0.0.1 not authorized to edit user ' + 'fred_user_id') + + ## START-AFTER + + def test_user_update_user_cannot_update_another_user(self): + '''Users should not be able to update other users' accounts.''' + + # 1. Setup. + + # Make a mock ckan.model.User object, Fred. + fred = factories.MockUser(name='fred') + + # Make a mock ckan.model object. + mock_model = mock.MagicMock() + # model.User.get(user_id) should return Fred. + mock_model.User.get.return_value = fred + + # Put the mock model in the context. + # This is easier than patching import ckan.model. + context = {'model': mock_model} + + # The logged-in user is going to be Bob, not Fred. + context['user'] = 'bob' + + # 2. Call the function that's being tested, once only. + + # Make Bob try to update Fred's user account. + params = { + 'id': fred.id, + 'name': 'updated_user_name', + } + result = helpers.call_auth('user_update', context=context, **params) + + # 3. Make assertions about the return value and/or side-effects. + + assert result['success'] is False + # FIXME: This error message should contain Fred's user name not his id. + assert result['msg'] == ('User bob not authorized to edit user ' + 'fred_user_id') + + # 4. Do nothing else! + + ## END-BEFORE + + def test_user_update_user_can_update_herself(self): + '''Users should be authorized to update their own accounts.''' + + # Make a mock ckan.model.User object, Fred. + fred = factories.MockUser(name='fred') + + # Make a mock ckan.model object. + mock_model = mock.MagicMock() + # model.User.get(user_id) should return our mock user. + mock_model.User.get.return_value = fred + + # Put the mock model in the context. + # This is easier than patching import ckan.model. + context = {'model': mock_model} + + # The 'user' in the context has to match fred.name, so that the + # auth function thinks that the user being updated is the same user as + # the user who is logged-in. + context['user'] = fred.name + + # Make Fred try to update his own user name. + params = { + 'id': fred.id, + 'name': 'updated_user_name', + } + result = helpers.call_auth('user_update', context=context, **params) + + assert result['success'] is True + + def test_user_update_with_no_user_in_context(self): + + # Make a mock ckan.model.User object. + mock_user = factories.MockUser(name='fred') + + # Make a mock ckan.model object. + mock_model = mock.MagicMock() + # model.User.get(user_id) should return our mock user. + mock_model.User.get.return_value = mock_user + + # Put the mock model in the context. + # This is easier than patching import ckan.model. + context = {'model': mock_model} + + # For this test we're going to have no 'user' in the context. + context['user'] = None + + params = { + 'id': mock_user.id, + 'name': 'updated_user_name', + } + result = helpers.call_auth('user_update', context=context, **params) + + assert result['success'] is False + # FIXME: Be nice if this error message was a complete sentence. + assert result['msg'] == 'Have to be logged in to edit user' + + # TODO: Tests for user_update's reset_key behavior. diff --git a/ckan/new_tests/logic/test_schema.py b/ckan/new_tests/logic/test_schema.py new file mode 100644 index 00000000000..61cf7a32bfe --- /dev/null +++ b/ckan/new_tests/logic/test_schema.py @@ -0,0 +1,11 @@ +''' + +We *don't* write tests for the schemas defined in :py:mod:`ckan.logic.schema`. +The validation done by the schemas is instead tested indirectly by the action +function tests. The reason for this is that CKAN actually does validation in +multiple places: some validation is done using schemas, some validation is done +in the action functions themselves, some is done in dictization, and some in +the model. By testing all the different valid and invalid inputs at the action +function level, we catch it all in one place. + +''' diff --git a/ckan/new_tests/logic/test_validators.py b/ckan/new_tests/logic/test_validators.py new file mode 100644 index 00000000000..4aab0fad23c --- /dev/null +++ b/ckan/new_tests/logic/test_validators.py @@ -0,0 +1,319 @@ +# -*- coding: utf-8 -*- +'''Unit tests for ckan/logic/validators.py. + +''' +import copy + +import mock +import nose.tools + +import ckan.new_tests.factories as factories +# Import some test helper functions from another module. +# This is bad (test modules shouldn't share code with eachother) but because of +# the way validator functions are organised in CKAN (in different modules in +# different places in the code) we have to either do this or introduce a shared +# test helper functions module (which we also don't want to do). +import ckan.new_tests.lib.navl.test_validators as t + + +def returns_arg(function): + '''A decorator that tests that the decorated function returns the argument + that it is called with, unmodified. + + :param function: the function to decorate + :type function: function + + Usage: + + @returns_arg + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(arg, context=None): + if context is None: + context = {} + result = function(arg, context=context) + assert result == arg, ( + 'Should return the argument that was passed to it, unchanged ' + '({arg})'.format(arg=repr(arg))) + return result + return call_and_assert + + +def raises_Invalid(function): + '''A decorator that asserts that the decorated function raises + dictization_functions.Invalid. + + Usage: + + @raises_Invalid + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(*args, **kwargs): + import ckan.lib.navl.dictization_functions as df + nose.tools.assert_raises(df.Invalid, function, *args, **kwargs) + return call_and_assert + + +def does_not_modify_other_keys_in_errors_dict(validator): + '''A decorator that asserts that the decorated validator doesn't add, + modify the value of, or remove any other keys from its ``errors`` dict + param. + + The function *may* modify its own errors dict key. + + :param validator: the validator function to decorate + :type validator: function + + Usage: + + @does_not_modify_other_keys_in_errors_dict + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def call_and_assert(key, data, errors, context=None): + if context is None: + context = {} + original_data = copy.deepcopy(data) + original_errors = copy.deepcopy(errors) + original_context = copy.deepcopy(context) + + result = validator(key, data, errors, context=context) + + # The validator function is allowed to modify its own key, so remove + # that key from both dicts for the purposes of the assertions below. + if key in errors: + del errors[key] + if key in original_errors: + del original_errors[key] + + assert errors.keys() == original_errors.keys(), ( + 'Should not add or remove keys from errors dict when called with ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + for key_ in errors: + assert errors[key_] == original_errors[key_], ( + 'Should not modify other keys in errors dict when called with ' + 'key: {key}, data: {data}, errors: {errors}, ' + 'context: {context}'.format(key=key, data=original_data, + errors=original_errors, + context=original_context)) + return result + return call_and_assert + + +def adds_message_to_errors_dict(error_message): + '''A decorator that asserts the the decorated validator adds a given + error message to the `errors` dict. + + :param error_message: the error message that the validator is expected to + add to the `errors` dict + :type error_message: string + + Usage: + + @adds_message_to_errors_dict('That login name is not available.') + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors) + + ''' + def decorator(validator): + def call_and_assert(key, data, errors, context): + result = validator(key, data, errors, context) + assert errors[key] == [error_message], ( + 'Should add message to errors dict: {msg}'.format( + msg=error_message)) + return result + return call_and_assert + return decorator + + +class TestValidators(object): + + def test_name_validator_with_invalid_value(self): + '''If given an invalid value name_validator() should do raise Invalid. + + ''' + import ckan.logic.validators as validators + import ckan.model as model + + invalid_values = [ + # Non-string names aren't allowed as names. + 13, + 23.7, + 100L, + 1.0j, + None, + True, + False, + ('a', 2, False), + [13, None, True], + {'foo': 'bar'}, + lambda x: x**2, + + # Certain reserved strings aren't allowed as names. + 'new', + 'edit', + 'search', + + # Strings < 2 characters long aren't allowed as names. + '', + 'a', + '2', + + # Strings > PACKAGE_NAME_MAX_LENGTH long aren't allowed as names. + 'a' * (model.PACKAGE_NAME_MAX_LENGTH + 1), + + # Strings containing non-ascii characters aren't allowed as names. + u"fred_❤%'\"Ußabc@fred.com", + + # Strings containing upper-case characters aren't allowed as names. + 'seanH', + + # Strings containing spaces aren't allowed as names. + 'sean h', + + # Strings containing punctuation aren't allowed as names. + 'seanh!', + ] + + for invalid_value in invalid_values: + @raises_Invalid + def call_validator(*args, **kwargs): + return validators.name_validator(*args, **kwargs) + call_validator(invalid_value, context={}) + + def test_name_validator_with_valid_value(self): + '''If given a valid string name_validator() should do nothing and + return the string. + + ''' + import ckan.logic.validators as validators + import ckan.model as model + + valid_names = [ + 'fred', + 'fred-flintstone', + 'fred_flintstone', + 'fred_flintstone-9', + 'f' * model.PACKAGE_NAME_MAX_LENGTH, + '-' * model.PACKAGE_NAME_MAX_LENGTH, + '_' * model.PACKAGE_NAME_MAX_LENGTH, + '9' * model.PACKAGE_NAME_MAX_LENGTH, + '99', + '--', + '__', + u'fred-flintstone_9', + ] + + for valid_name in valid_names: + @returns_arg + def call_validator(*args, **kwargs): + return validators.name_validator(*args, **kwargs) + call_validator(valid_name) + + ## START-AFTER + + def test_user_name_validator_with_non_string_value(self): + '''user_name_validator() should raise Invalid if given a non-string + value. + + ''' + import ckan.logic.validators as validators + + non_string_values = [ + 13, + 23.7, + 100L, + 1.0j, + None, + True, + False, + ('a', 2, False), + [13, None, True], + {'foo': 'bar'}, + lambda x: x**2, + ] + + # Mock ckan.model. + mock_model = mock.MagicMock() + # model.User.get(some_user_id) needs to return None for this test. + mock_model.User.get.return_value = None + + key = ('name',) + for non_string_value in non_string_values: + data = factories.validator_data_dict() + data[key] = non_string_value + errors = factories.validator_errors_dict() + errors[key] = [] + + @t.does_not_modify_data_dict + @raises_Invalid + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors, context={'model': mock_model}) + + ## END-BEFORE + + def test_user_name_validator_with_a_name_that_already_exists(self): + '''user_name_validator() should add to the errors dict if given a + user name that already exists. + + ''' + import ckan.logic.validators as validators + + # Mock ckan.model. model.User.get('user_name') will return another mock + # object rather than None, which will simulate an existing user with + # the same user name in the database. + mock_model = mock.MagicMock() + + data = factories.validator_data_dict() + key = ('name',) + data[key] = 'user_name' + errors = factories.validator_errors_dict() + errors[key] = [] + + @does_not_modify_other_keys_in_errors_dict + @t.does_not_modify_data_dict + @t.returns_None + @adds_message_to_errors_dict('That login name is not available.') + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors, context={'model': mock_model}) + + def test_user_name_validator_successful(self): + '''user_name_validator() should do nothing if given a valid name.''' + + import ckan.logic.validators as validators + + data = factories.validator_data_dict() + key = ('name',) + data[key] = 'new_user_name' + errors = factories.validator_errors_dict() + errors[key] = [] + + # Mock ckan.model. + mock_model = mock.MagicMock() + # model.User.get(user_name) should return None, to simulate that no + # user with that name exists in the database. + mock_model.User.get.return_value = None + + @t.does_not_modify_errors_dict + @t.does_not_modify_data_dict + @t.returns_None + def call_validator(*args, **kwargs): + return validators.user_name_validator(*args, **kwargs) + call_validator(key, data, errors, context={'model': mock_model}) + + # TODO: Test user_name_validator()'s behavior when there's a 'user_obj' in + # the context dict. diff --git a/ckan/new_tests/migration/__init__.py b/ckan/new_tests/migration/__init__.py new file mode 100644 index 00000000000..bcc2c6ca717 --- /dev/null +++ b/ckan/new_tests/migration/__init__.py @@ -0,0 +1,8 @@ +'''**All migration scripts should have tests.** + +.. todo:: + + Write some tests for a migration script, and then use them as an example to + fill out this guidelines section. + +''' diff --git a/ckan/new_tests/model/__init__.py b/ckan/new_tests/model/__init__.py new file mode 100644 index 00000000000..3e31913cf96 --- /dev/null +++ b/ckan/new_tests/model/__init__.py @@ -0,0 +1,9 @@ +'''**All model methods should have tests**. + +.. todo:: + + Write the tests for one ``ckan.model`` module, figuring out the best way + to write model tests. Then fill in this guidelines section, using the first + set of model tests as an example. + +''' diff --git a/ckan/new_tests/plugins/__init__.py b/ckan/new_tests/plugins/__init__.py new file mode 100644 index 00000000000..8cdf1c240a1 --- /dev/null +++ b/ckan/new_tests/plugins/__init__.py @@ -0,0 +1,25 @@ +'''The plugin interfaces in :mod:`ckan.plugins.interfaces` are not directly +testable because they don't contain any code, *but*: + +* Each plugin interface should have an example plugin in :mod:`ckan.ckanext` + and the example plugin should have its own functional tests. + +* The tests for the code that calls the plugin interface methods should test + that the methods are called correctly. + +For example :func:`ckan.logic.action.get.package_show` calls +:meth:`ckan.plugins.interfaces.IDatasetForm.read`, so the +:func:`~ckan.logic.action.get.package_show` tests should include tests +that :meth:`~ckan.plugins.interfaces.IDatasetForm.read` is called at the +right times and with the right parameters. + +Everything in :mod:`ckan.plugins.toolkit` should have tests, because these +functions are part of the API for extensions to use. But +:mod:`~ckan.plugins.toolkit` imports most of these functions from elsewhere +in CKAN, so the tests should be elsewhere also, in the test modules for the +modules where the functions are defined. + +Other than the plugin interfaces and plugins toolkit, any other code in +:mod:`ckan.plugins` should have tests. + +''' diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index a75d6e99906..74462835474 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -578,7 +578,7 @@ def user_create(context, data_dict=None): be decorated with the ``auth_allow_anonymous_access`` decorator, available on the plugins toolkit. - For example: + For example:: import ckan.plugins as p diff --git a/ckan/tests/functional/api/test_activity.py b/ckan/tests/functional/api/test_activity.py index fbabde25220..bd42409edd6 100644 --- a/ckan/tests/functional/api/test_activity.py +++ b/ckan/tests/functional/api/test_activity.py @@ -875,54 +875,6 @@ def _update_group(self, group, user): assert timestamp >= before['time'] and timestamp <= after['time'], \ str(activity['timestamp']) - def _update_user(self, user): - """ - Update the given user and test that the correct activity stream item - and detail are emitted. - - """ - response = self.app.post('/api/action/user_show', - json.dumps({'id': user['id']}), - extra_environ={'Authorization': str(user['apikey'])}) - response_dict = json.loads(response.body) - assert response_dict['success'] is True - user_dict = response_dict['result'] - - before = self.record_details(user_dict['id'], - apikey=user_dict['apikey']) - - # Update the user. - user_dict['about'] = 'edited' - if not user_dict.get('email'): - user_dict['email'] = 'there has to be a value in email' - self.app.post('/api/action/user_update', json.dumps(user_dict), - extra_environ={'Authorization': str(user['apikey'])}) - - after = self.record_details(user_dict['id'], - apikey=user_dict['apikey']) - - # Find the new activity. - new_activities = find_new_activities(before['user activity stream'], - after['user activity stream']) - assert len(new_activities) == 1, ("There should be 1 new activity in " - "the user's activity stream, but found %i" % len(new_activities)) - activity = new_activities[0] - - # Check that the new activity has the right attributes. - assert activity['object_id'] == user_dict['id'], ( - str(activity['object_id'])) - assert activity['user_id'] == user_dict['id'], str(activity['user_id']) - assert activity['activity_type'] == 'changed user', ( - str(activity['activity_type'])) - if 'id' not in activity: - assert False, "activity object has no id value" - # TODO: Test for the _correct_ revision_id value. - if 'revision_id' not in activity: - assert False, "activity has no revision_id value" - timestamp = datetime_from_string(activity['timestamp']) - assert timestamp >= before['time'] and timestamp <= after['time'], \ - str(activity['timestamp']) - def _delete_resources(self, package): """ Remove all resources (if any) from the given package, and test that @@ -1488,17 +1440,6 @@ def test_create_user(self): assert len(details) == 0, ("There shouldn't be any activity details" " for a 'new user' activity") - def test_update_user(self): - """ - Test updated user activity stream. - - Test that correct activity stream item is created when users are - updated. - - """ - for user in self.users: - self._update_user(user) - def test_create_group(self): user = self.normal_user diff --git a/ckan/tests/functional/api/test_user.py b/ckan/tests/functional/api/test_user.py index 06dd35dd7fc..317428de865 100644 --- a/ckan/tests/functional/api/test_user.py +++ b/ckan/tests/functional/api/test_user.py @@ -80,23 +80,3 @@ def test_user_create_simple(self): assert 'email' in user_dict assert 'apikey' in user_dict assert 'password' not in user_dict - - def test_user_update_simple(self): - '''Simple update of a user by themselves.''' - context = { - 'model': model, - 'session': model.Session, - 'user': 'annafan', - } - - data_dict = { - 'id': 'annafan', - 'email': 'anna@example.com', - } - - user_dict = logic.get_action('user_update')(context, data_dict) - - assert_equal(user_dict['email'], 'anna@example.com') - assert 'apikey' in user_dict - assert 'password' not in user_dict - diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index 01847884c8c..57f29963910 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -1139,11 +1139,11 @@ def test_22_user_dictize_as_sysadmin(self): # Check sensitive data is available assert 'apikey' in user_dict - assert 'reset_key' in user_dict assert 'email' in user_dict - # Passwords should never be available + # Passwords and reset keys should never be available assert 'password' not in user_dict + assert 'reset_key' not in user_dict def test_23_user_dictize_as_same_user(self): '''User should be able to see their own sensitive data.''' @@ -1163,11 +1163,11 @@ def test_23_user_dictize_as_same_user(self): # Check sensitive data is available assert 'apikey' in user_dict - assert 'reset_key' in user_dict assert 'email' in user_dict - # Passwords should never be available + # Passwords and reset keys should never be available assert 'password' not in user_dict + assert 'reset_key' not in user_dict def test_24_user_dictize_as_other_user(self): '''User should not be able to see other's sensitive data.''' diff --git a/ckan/tests/lib/test_navl.py b/ckan/tests/lib/test_navl.py index 758c8a049a0..7536ec7c39d 100644 --- a/ckan/tests/lib/test_navl.py +++ b/ckan/tests/lib/test_navl.py @@ -198,20 +198,6 @@ def test_default(): assert converted_data == {('1',): 'default', ('0',): '0 value'}, converted_data -def test_ignore_missing(): - schema = { - "__junk": [ignore], - "__extras": [ignore, default("weee")], - "0": [default("default")], - "1": [ignore_missing, default("default")], - } - - converted_data, errors = validate_flattened(data, schema) - - assert not errors - assert converted_data == {('0',): '0 value'}, converted_data - - def test_flatten(): data = {'extras': [{'key': 'genre', 'value': u'horror'}, diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index d5709e7030d..c116f522abd 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -378,7 +378,6 @@ def test_05_user_show(self): result = res_obj['result'] assert result['name'] == 'annafan' assert 'apikey' in result - assert 'reset_key' in result # Sysadmin user can see everyone's api key res = self.app.post('/api/action/user_show', params=postparams, @@ -388,7 +387,6 @@ def test_05_user_show(self): result = res_obj['result'] assert result['name'] == 'annafan' assert 'apikey' in result - assert 'reset_key' in result def test_05_user_show_edits(self): postparams = '%s=1' % json.dumps({'id':'tester'}) @@ -455,113 +453,6 @@ def test_11_user_create_wrong_password(self): assert res_obj['error'] == { '__type': 'Validation Error', 'password': ['Your password must be 4 characters or longer']} - def test_12_user_update(self): - normal_user_dict = {'id': self.normal_user.id, - 'name': self.normal_user.name, - 'fullname': 'Updated normal user full name', - 'email': 'me@test.org', - 'about':'Updated normal user about'} - - sysadmin_user_dict = {'id': self.sysadmin_user.id, - 'fullname': 'Updated sysadmin user full name', - 'email': 'me@test.org', - 'about':'Updated sysadmin user about'} - - #Normal users can update themselves - postparams = '%s=1' % json.dumps(normal_user_dict) - res = self.app.post('/api/action/user_update', params=postparams, - extra_environ={'Authorization': str(self.normal_user.apikey)}) - - res_obj = json.loads(res.body) - assert res_obj['help'].startswith("Update a user account.") - assert res_obj['success'] == True - result = res_obj['result'] - assert result['id'] == self.normal_user.id - assert result['name'] == self.normal_user.name - assert result['fullname'] == normal_user_dict['fullname'] - assert result['about'] == normal_user_dict['about'] - assert 'apikey' in result - assert 'created' in result - assert 'display_name' in result - assert 'number_administered_packages' in result - assert 'number_of_edits' in result - assert not 'password' in result - - #Sysadmin users can update themselves - postparams = '%s=1' % json.dumps(sysadmin_user_dict) - res = self.app.post('/api/action/user_update', params=postparams, - extra_environ={'Authorization': str(self.sysadmin_user.apikey)}) - - res_obj = json.loads(res.body) - assert res_obj['help'].startswith("Update a user account.") - assert res_obj['success'] == True - result = res_obj['result'] - assert result['id'] == self.sysadmin_user.id - assert result['name'] == self.sysadmin_user.name - assert result['fullname'] == sysadmin_user_dict['fullname'] - assert result['about'] == sysadmin_user_dict['about'] - - #Sysadmin users can update all users - postparams = '%s=1' % json.dumps(normal_user_dict) - res = self.app.post('/api/action/user_update', params=postparams, - extra_environ={'Authorization': str(self.sysadmin_user.apikey)}) - - res_obj = json.loads(res.body) - assert res_obj['help'].startswith("Update a user account.") - assert res_obj['success'] == True - result = res_obj['result'] - assert result['id'] == self.normal_user.id - assert result['name'] == self.normal_user.name - assert result['fullname'] == normal_user_dict['fullname'] - assert result['about'] == normal_user_dict['about'] - - #Normal users can not update other users - postparams = '%s=1' % json.dumps(sysadmin_user_dict) - res = self.app.post('/api/action/user_update', params=postparams, - extra_environ={'Authorization': str(self.normal_user.apikey)}, - status=StatusCodes.STATUS_403_ACCESS_DENIED) - - res_obj = json.loads(res.body) - assert res_obj['help'].startswith("Update a user account.") - assert res_obj['error'] == { - '__type': 'Authorization Error', - 'message': 'Access denied' - } - assert res_obj['success'] is False - - def test_12_user_update_errors(self): - test_calls = ( - # Empty name - {'user_dict': {'id': self.normal_user.id, - 'name':'', - 'email':'test@test.com'}, - 'messages': [('name','Name must be at least 2 characters long')]}, - - # Invalid characters in name - {'user_dict': {'id': self.normal_user.id, - 'name':'i++%', - 'email':'test@test.com'}, - 'messages': [('name','Url must be purely lowercase alphanumeric')]}, - # Existing name - {'user_dict': {'id': self.normal_user.id, - 'name':self.sysadmin_user.name, - 'email':'test@test.com'}, - 'messages': [('name','That login name is not available')]}, - # Missing email - {'user_dict': {'id': self.normal_user.id, - 'name':self.normal_user.name}, - 'messages': [('email','Missing value')]}, - ) - - for test_call in test_calls: - postparams = '%s=1' % json.dumps(test_call['user_dict']) - res = self.app.post('/api/action/user_update', params=postparams, - extra_environ={'Authorization': str(self.normal_user.apikey)}, - status=StatusCodes.STATUS_409_CONFLICT) - res_obj = json.loads(res.body) - for expected_message in test_call['messages']: - assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) - def test_13_group_list(self): postparams = '%s=1' % json.dumps({}) res = self.app.post('/api/action/group_list', params=postparams) diff --git a/dev-requirements.txt b/dev-requirements.txt index 2c1c6cb5041..b55719c6388 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -7,3 +7,5 @@ httpretty==0.6.2 pep8==1.4.6 Sphinx==1.1.3 polib==1.0.3 +mock==1.0.1 +factory-boy==2.1.1 diff --git a/doc/multilingual.rst b/doc/multilingual.rst index 533df5484bf..32a5b7bb847 100644 --- a/doc/multilingual.rst +++ b/doc/multilingual.rst @@ -36,4 +36,4 @@ If you have a source installation of CKAN you can test the multilingual extensio nosetests --ckan ckanext/multilingual/tests -See :ref:`basic-tests` for more information. +See :doc:`test` for more information. diff --git a/doc/test.rst b/doc/test.rst index f74eceb2cb2..cdd9b928e0a 100644 --- a/doc/test.rst +++ b/doc/test.rst @@ -6,11 +6,10 @@ If you're a CKAN developer, if you're developing an extension for CKAN, or if you're just installing CKAN from source, you should make sure that CKAN's tests pass for your copy of CKAN. This section explains how to run CKAN's tests. -.. _basic-tests: ----------------------------------- -Installing Additional Dependencies ----------------------------------- +------------------------------- +Install additional dependencies +------------------------------- Some additional dependencies are needed to run the tests. Make sure you've created a config file at |development.ini|, then activate your @@ -31,45 +30,10 @@ environment: pip install -r |virtualenv|/src/ckan/dev-requirements.txt -------------------- -Testing with SQLite -------------------- -To run the CKAN tests using SQLite as the database library: - -.. parsed-literal:: - - cd |virtualenv|/src/ckan - nosetests --ckan ckan - -You *must* run the tests from the CKAN directory as shown above, otherwise the -``--ckan`` plugin won't work correctly. - -In deployment CKAN uses PostgreSQL, not SQLite. Running the tests with SQLite -is less thorough but much quicker than with PostgreSQL, good enough for an -initial check but you should run the tests with PostgreSQL before deploying -anything or releasing any code. - -Testing Core Extensions -======================= - -CKAN's core extensions (those extensions that are kept in the CKAN codebase -alongside CKAN itself) have their own tests. For example, to run the tests for -the stats extension do:: - - nosetests --ckan ckanext/stats - -To run the tests for all of the core extensions at once:: - - nosetests --ckan ckanext - -Or to run the CKAN tests and the core extensions tests together:: - - nosetests --ckan ckan ckanext - ------------------------ -Testing with PostgreSQL ------------------------ +------------------------- +Set up the test databases +------------------------- .. versionchanged:: 2.1 Previously |postgres| tests used the databases defined in your @@ -86,24 +50,26 @@ Create test databases: This database connection is specified in the ``test-core.ini`` file by the ``sqlalchemy.url`` parameter. -CKAN's default nose configuration file (``test.ini``) specifies SQLite as the -database library (it also sets ``faster_db_test_hacks``). To run the tests more -thoroughly with PostgreSQL, specify the ``test-core.ini`` nose configuration -file instead, for example:: - nosetests --ckan --with-pylons=test-core.ini ckan - nosetests --ckan --with-pylons=test-core.ini ckanext/stats - nosetests --ckan --with-pylons=test-core.ini ckanext +------------- +Run the tests +------------- + +To run CKAN's tests using PostgreSQL as the database, you have to give the +``--with-pylons=test-core.ini`` option on the command line. This command will +run the tests for CKAN core and for the core extensions:: + nosetests --ckan --with-pylons=test-core.ini ckan ckanext The speed of the PostgreSQL tests can be improved by running PostgreSQL in memory and turning off durability, as described `in the PostgreSQL documentation `_. + .. _migrationtesting: ----------------- -Migration Testing +Migration testing ----------------- If you're a CKAN developer or extension developer and your new code requires a @@ -111,7 +77,7 @@ change to CKAN's model, you'll need to write a migration script. To ensure that the migration script itself gets tested, you should run the tests with they ``--ckan-migration`` option, for example:: - nosetests --ckan --ckan-migration --with-pylons=test-core.ini ckan + nosetests --ckan --ckan-migration --with-pylons=test-core.ini ckan ckanext By default tests are run using the model defined in ``ckan/model``. With the ``--ckan-migration`` option the tests will run using a database that diff --git a/doc/testing-coding-standards.rst b/doc/testing-coding-standards.rst new file mode 100644 index 00000000000..8e0b7fd4349 --- /dev/null +++ b/doc/testing-coding-standards.rst @@ -0,0 +1,445 @@ +======================== +Testing coding standards +======================== + +**All new code, or changes to existing code, should have new or updated tests +before being merged into master**. This document gives some guidelines for +developers who are writing tests or reviewing code for CKAN. + + +-------------------------------------- +Transitioning from legacy to new tests +-------------------------------------- + +CKAN is an old code base with a large legacy test suite in +:mod:`ckan.tests`. The legacy tests are difficult to maintain and +extend, but are too many to be replaced all at once in a single effort. So +we're following this strategy: + +#. A new test suite has been started in :mod:`ckan.new_tests`. +#. For now, we'll run both the legacy tests and the new tests before + merging something into the master branch. +#. Whenever we add new code, or change existing code, we'll add new-style tests + for it. +#. If you change the behavior of some code and break some legacy tests, + consider adding new tests for that code and deleting the legacy tests, + rather than updating the legacy tests. +#. Now and then, we'll write a set of new tests to cover part of the code, + and delete the relevant legacy tests. For example if you want to refactor + some code that doesn't have good tests, write a set of new-style tests for + it first, refactor, then delete the relevant legacy tests. + +In this way we can incrementally extend the new tests to cover CKAN one "island +of code" at a time, and eventually we can delete the legacy :mod:`ckan.tests` +directory entirely. + + +-------------------------------------- +Guidelines for writing new-style tests +-------------------------------------- + +We want the tests in :mod:`ckan.new_tests` to be: + +Fast + * 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). + + Instead write helper functions that create test objects and return them, + 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. + + * Tests shouldn't be tightly coupled to each other, changing a test shouldn't + affect other tests. + +Clear + It should be quick and easy to see what went wrong when a test fails, or + to see what a test does and how it works if you have to debug or update + a test. + + You shouldn't have to figure out what a complex test method does, or go and + look up a lot of code in other files to understand a test method. + + * Tests should follow the canonical form for a unit test, see + :ref:`test recipe`. + + * Write lots of small, simple test methods not a few big, complex tests. + + * Each test method should test just One Thing. + + * The name of a test method should clearly explain the intent of the test. + See :ref:`naming`. + + * Test methods and helper functions should have docstrings. + +Easy to find + It should be easy to know where to add new tests for some new or changed + code, or to find the existing tests for some code. + + * See :ref:`organization` + + * See :ref:`naming`. + +Easy to write + Writing lots of small, clear and simple tests that all follow similar + recipes and organization should make tests easy to write, as well as easy + to read. + +The follow sections give some more specific guidelines and tips for writing +CKAN tests. + + +.. _organization: + +How should tests be organized? +============================== + +The organization of test modules in :mod:`ckan.new_tests` mirrors the +organization of the source modules in :mod:`ckan`:: + + ckan/ + tests/ + controllers/ + test_package.py <-- Tests for ckan/controllers/package.py + ... + lib/ + test_helpers.py <-- Tests for ckan/lib/helpers.py + ... + logic/ + action/ + test_get.py + ... + auth/ + test_get.py + ... + test_converters.py + test_validators.py + migration/ + versions/ + test_001_add_existing_tables.py + ... + model/ + test_package.py + ... + ... + +There are a few exceptional test modules that don't fit into this structure, +for example PEP8 tests and coding standards tests. These modules can just go in +the top-level ``ckan/new_tests/`` directory. There shouldn't be too many of these. + + +.. _naming: + +Naming test methods +------------------- + +`The name of a test method should clearly explain the intent of the test `_. + +Test method names are printed out when tests fail, so the user can often +see what went wrong without having to look into the test file. When they +do need to look into the file to debug or update a test, the test name +helps to clarify the test. + +Do this even if it means your method name gets really long, since we don't +write code that calls our test methods there's no advantage to having short +test method names. + +Some modules in CKAN contain large numbers of loosely related functions. +For example, :mod:`ckan.logic.action.update` contains all functions for +updating things in CKAN. This means that +:mod:`ckan.new_tests.logic.action.test_update` is going to contain an even larger +number of test functions. + +So as well as the name of each test method explaining the intent of the test, +it's important to name the test function after the function it's testing, for +example all the tests for ``user_update`` should be named +``test_user_update_*``. + +Good test names: + +* ``test_user_update_with_id_that_does_not_exist`` +* ``test_user_update_with_no_id`` +* ``test_user_update_with_invalid_name`` + +Bad test names: + +* ``test_user_update`` +* ``test_update_pkg_1`` +* ``test_package`` + +.. _test recipe: + +Recipe for a test method +======================== + +The `Pylons Unit Testing Guidelines `_ +give the following recipe for all unit test methods to follow: + +1. Set up the preconditions for the method / function being tested. +2. Call the method / function exactly one time, passing in the values + established in the first step. +3. Make assertions about the return value, and / or any side effects. +4. Do absolutely nothing else. + +Most CKAN tests should follow this form. Here's an example of a simple action +function test demonstrating the recipe: + +.. literalinclude:: ../ckan/new_tests/logic/action/test_update.py + :start-after: ## START-AFTER + :end-before: ## END-BEFORE + +One common exception is when you want to use a ``for`` loop to call the +function being tested multiple times, passing it lots of different arguments +that should all produce the same return value and/or side effects. For example, +this test from :py:mod:`ckan.new_tests.logic.action.test_update`: + +.. literalinclude:: ../ckan/new_tests/logic/action/test_update.py + :start-after: ## START-FOR-LOOP-EXAMPLE + :end-before: ## END-FOR-LOOP-EXAMPLE + +The behavior of :py:func:`~ckan.logic.action.update.user_update` is the same +for every invalid value. +We do want to test :py:func:`~ckan.logic.action.update.user_update` with lots +of different invalid names, but we obviously don't want to write a dozen +separate test methods that are all the same apart from the value used for the +invalid user name. We don't really want to define a helper method and a dozen +test methods that call it either. So we use a simple loop. Technically this +test calls the function being tested more than once, but there's only one line +of code that calls it. + + +How detailed should tests be? +============================= + +Generally, what we're trying to do is test the *interfaces* between modules in +a way that supports modularization: if you change the code within a function, +method, class or module, if you don't break any of that code's tests you +should be able to expect that CKAN as a whole will not be broken. + +As a general guideline, the tests for a function or method should: + +- Test for success: + + - Test the function with typical, valid input values + - Test with valid, edge-case inputs + - If the function has multiple parameters, test them in different + combinations + +- Test for failure: + + - Test that the function fails correctly (e.g. raises the expected type of + exception) when given likely invalid inputs (for example, if the user + passes an invalid user_id as a parameter) + - Test that the function fails correctly when given bizarre input + +- Test that the function behaves correctly when given unicode characters as + input + +- Cover the interface of the function: test all the parameters and features of + the function + + +.. _factory-boy: + +Creating test objects: :py:mod:`ckan.new_tests.factories` +--------------------------------------------------------- + +.. automodule:: ckan.new_tests.factories + :members: + + +Test helper functions: :mod:`ckan.new_tests.helpers` +---------------------------------------------------- + +.. automodule:: ckan.new_tests.helpers + :members: + + +.. _mock: + +Mocking: the ``mock`` library +----------------------------- + +We use the `mock library `_ to +replace parts of CKAN with mock objects. This allows a CKAN +function to be tested independently of other parts of CKAN or third-party +libraries that the function uses. This generally makes the test simpler and +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: + +.. code-block:: python + + >>> import mock + >>> my_mock = mock.MagicMock() + >>> my_mock.foo + + >>> my_mock.bar + + >>> my_mock.foobar() + + >>> my_mock.foobar(1, 2, 'barfoo') + + +When a test needs a mock object to actually have some behavior besides always +returning other mock objects, it can set the value of a certain attribute on +the mock object, set the return value of a certain method, specify that a +certain method should raise a certain exception, etc. + +You should read the mock library's documentation to really understand what's +going on, but here's an example of a test from +:py:mod:`ckan.new_tests.logic.auth.test_update` that tests the +:py:func:`~ckan.logic.auth.update.user_update` authorization function and mocks +out :py:mod:`ckan.model`: + +.. literalinclude:: ../ckan/new_tests/logic/auth/test_update.py + :start-after: ## START-AFTER + :end-before: ## END-BEFORE + +---- + +The following sections will give specific guidelines and examples for writing +tests for each module in CKAN. + +.. note:: + + When we say that *all* functions should have tests in the sections below, we + mean all *public* functions that the module or class exports for use by + other modules or classes in CKAN or by extensions or templates. + + *Private* helper methods (with names beginning with ``_``) never have to + have their own tests, although they can have tests if helpful. + +Writing :mod:`ckan.logic.action` tests +-------------------------------------- + +.. automodule:: ckan.new_tests.logic.action + + +Writing :mod:`ckan.logic.auth` tests +------------------------------------ + +.. automodule:: ckan.new_tests.logic.auth + + +Writing converter and validator tests +------------------------------------- + +**All converter and validator functions should have unit tests.** + +Although these converter and validator functions are tested indirectly by the +action function tests, this may not catch all the converters and validators and +all their options, and converters and validators are not only used by the +action functions but are also available to plugins. Having unit tests will also +help to clarify the intended behavior of each converter and validator. + +CKAN's action functions call +:py:func:`ckan.lib.navl.dictization_functions.validate` to validate data posted +by the user. Each action function passes a schema from +:py:mod:`ckan.logic.schema` to +:py:func:`~ckan.lib.navl.dictization_functions.validate`. The schema gives +:py:func:`~ckan.lib.navl.dictization_functions.validate` lists of validation +and conversion functions to apply to the user data. These validation and +conversion functions are defined in :py:mod:`ckan.logic.validators`, +:py:mod:`ckan.logic.converters` and :py:mod:`ckan.lib.navl.validators`. + +Most validator and converter tests should be unit tests that test the validator +or converter function in isolation, without bringing in other parts of CKAN or +touching the database. This requires using the ``mock`` library to mock +``ckan.model``, see :ref:`mock`. + +When testing validators, we often want to make the same assertions in many +tests: assert that the validator didn't modify the ``data`` dict, assert that +the validator didn't modify the ``errors`` dict, assert that the validator +raised ``Invalid``, etc. Decorator functions are defined at the top of +validator test modules like :py:mod:`ckan.new_tests.logic.test_validators` to +make these common asserts easy. To use one of these decorators you have to: + +1. Define a nested function inside your test method, that simply calls the + validator function that you're trying to test. +2. Apply the decorators that you want to this nested function. +3. Call the nested function. + +Here's an example of a simple validator test that uses this technique: + +.. literalinclude:: ../ckan/new_tests/logic/test_validators.py + :start-after: ## START-AFTER + :end-before: ## END-BEFORE + + +No tests for :mod:`ckan.logic.schema.py` +---------------------------------------- + +.. automodule:: ckan.new_tests.logic.test_schema + + +Writing :mod:`ckan.controllers` tests +------------------------------------- + +.. automodule:: ckan.new_tests.controllers + + +Writing :mod:`ckan.model` tests +------------------------------- + +.. automodule:: ckan.new_tests.model + + +Writing :mod:`ckan.lib` tests +----------------------------- + +.. automodule:: ckan.new_tests.lib + + +Writing :mod:`ckan.plugins` tests +--------------------------------- + +.. automodule:: ckan.new_tests.plugins + + +Writing :mod:`ckan.migration` tests +----------------------------------- + +.. automodule:: ckan.new_tests.migration + + +Writing :mod:`ckan.ckanext` tests +--------------------------------- + +Within extensions, follow the same guidelines as for CKAN core. For example if +an extension adds an action function then the action function should have +tests, etc.