Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ideas for improving tests that use auth tokens #58

Closed
jc0n opened this issue Dec 9, 2011 · 1 comment
Closed

Ideas for improving tests that use auth tokens #58

jc0n opened this issue Dec 9, 2011 · 1 comment
Assignees
Labels

Comments

@jc0n
Copy link
Member

jc0n commented Dec 9, 2011

I came up with two ideas for reducing the amount of token passing required for the service methods and improving readability of manager tests. Conceptually, it makes sense to think of the service methods as operations on a context where the auth_token defines that context. It follows that the context should be established as a pre-condition to those operations and all subsequent operations perform within that context. In most cases the same auth token is used for multiple operations.

First Idea

Accept an auth_token as a constructor argument in ObjectManager that way unless one is specified as a keyword argument to the service method, the default (constructor argument) is used. Usage of the managers would then look a bit like..

user_manager = managers.UserManager(default_token)
user = user_manager.create('username', 'first_name', ... ) # create with user privileges
user_manager.update(user.id, {'first_name': 'joe'})
user_manager.delete(user.id)

admin_user_manager = managers.UserManager(admin_token)
admin_user_manager.create('otheruser', 'other_name'...) # create with admin privileges

Second idea

Slightly more "magic" than above but has the benefit of being a drop in solution that doesn't have to be applied everywhere. It also wont require changes in object managers; only the call site of the service methods. The other benefit is that you can have the same manager instance decoupled from the auth token. My thoughts on this were that test cases which must perform an operation (ie. to seed test data) setup and call self.admin_abcxyz_manager which would be wrapped with an admin auth token.

I like this because it also helps with readability of the tests which could be drastically improved so that we know what we're actually testing. When you see a call to admin_abcxyz_manager you know it must succeed and that we are not testing the operation with this call. You also know that all calls to abcxyz_manager no longer assume the auth token is for admin which most currently do. The idea stems from cleanup I am working on for #39 to re-use all of the existing tests for verifying different roles. The tests will be written in a way which makes no assumption about the auth token being for the admin.

class ManagerAuthTokenWrapper(object):
    """
    Wrap ObjectManager methods to automatically provide a specified auth token
    """

    WRAP_METHODS = frozenset(('create', 'batch_create', 'update', 'get_filtered'))

    def __init__(self, manager, token_getter):
        assert isinstance(manager, ObjectManager)
        assert callable(token_getter)
        self.manager = manager
        self.token_getter = token_getter

    def _wrapped_method(self, method):
        @functools.wraps(method)
        def _wrapper(*args, **kwargs):
            if args and isinstance(args[0], facade.models.AuthToken):
                return method(*args, **kwargs)
            token = kwargs.pop('auth_token', self.token_getter())
            return method(token, *args, **kwargs)

        return _wrapper

    def __getattr__(self, name):
        attr = getattr(self.manager, name)
        if name in self.WRAP_METHODS:
            return self._wrapped_method(attr)
        else:
            return attr

Usage of this approach would make code like...

user_manager = ManagerAuthTokenWrapper(user_manager, default_token)
admin_user_manager = ManagerAuthTokenWrapper(user_manager, admin_token)

user = user_manager.create('username', 'first_name', ... ) # create with user privileges
user_manager.update(user.id, {'first_name': 'joe'})
user_manager.delete(user.id)
@ghost ghost assigned jc0n Dec 9, 2011
@jc0n
Copy link
Member Author

jc0n commented Jan 8, 2012

The wrapper was included as part of issue #40.

@jc0n jc0n closed this as completed Jan 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant