Merge branch '1117-start-new-test-suite'

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 <http://docs.ckan.org/en/latest/ckan-coding-standards.html>`_
-* `Python Coding Standards <http://docs.ckan.org/en/latest/python-coding-standards.html>`_
-* `HTML Coding Standards <http://docs.ckan.org/en/latest/html-coding-standards.html>`_
-* `CSS Coding Standards <http://docs.ckan.org/en/latest/css-coding-standards.html>`_
-* `JavaScript Coding Standards <http://docs.ckan.org/en/latest/javascript-coding-standards.html>`_
+   testing-coding-standards
+
+* `CKAN coding standards <http://docs.ckan.org/en/latest/ckan-coding-standards.html>`_
+* `Python coding standards <http://docs.ckan.org/en/latest/python-coding-standards.html>`_
+* `HTML coding standards <http://docs.ckan.org/en/latest/html-coding-standards.html>`_
+* `CSS coding standards <http://docs.ckan.org/en/latest/css-coding-standards.html>`_
+* `JavaScript coding standards <http://docs.ckan.org/en/latest/javascript-coding-standards.html>`_
+* `Testing coding standards <http://docs.ckan.org/en/latest/testing-coding-standards.html>`_
 
 
 ---------------

ckan/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):

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

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:

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):

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.
+
+'''

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']}