Fixed #20456: easier unit-testing for CBV #2368

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@Keats

Added a class method to return an instance of a CBV.
This allows to unit test part of a CBV directly, without using
the client.

Note: as the ticket didn't have much discussion going on, I did a POC (only modified a single test to show how it would work), feedback on it needed before going further

@benoitbryon

I am ok with the as_instance() technique. Thanks @Keats for this proposal :)

In my dreams, calling the class constructor is the obvious way to get an instance of the class... but I know it is not so easy with current CBV implementation... so having some "as_instance()" method looks consistent with "as_view".

@benoitbryon benoitbryon and 1 other commented on an outdated diff Mar 3, 2014
django/views/generic/base.py
@@ -76,6 +76,19 @@ def view(request, *args, **kwargs):
update_wrapper(view, cls.dispatch, assigned=())
return view
+ @classonlymethod
+ def as_instance(cls, request='', *args, **kwargs):
@benoitbryon
benoitbryon Mar 3, 2014

Can you explain why request='' by default? Is empty string a suitable default for a request instance?

@benoitbryon
benoitbryon Mar 3, 2014

What about having request as a required positional argument?

Or, if it is to support calls such as view = views.CustomTemplateView.as_instance() (no arguments), then what about initializing request like if request is None: request = RequestFactory().get('/fake')

@Keats
Keats Mar 5, 2014

I just didn't want to put that logic inside the as_instance, if people need a request they can provide one.

You don't always need a request depending on what you're testing so empty string would be ok

@benoitbryon
benoitbryon Mar 5, 2014

You don't always need a request depending on what you're testing...

Ok.

... so empty string would be ok

I'm not sure about it. If you do not want a default request instance, I'd prefer None instead of empty string.
That said, both None and empty string would trigger exceptions if an user uses view = CBV().as_instance() without passing a request, then he calls something that tries to use the request such as view.request.GET.

May someone else tell his opinion?

@benoitbryon benoitbryon commented on an outdated diff Mar 3, 2014
tests/generic_views/test_base.py
@@ -294,11 +294,14 @@ def test_extra_template_params(self):
"""
A template view can be customized to return extra context.
"""
- response = self.client.get('/template/custom/bar/')
- self.assertEqual(response.status_code, 200)
- self.assertEqual(response.context['foo'], 'bar')
- self.assertEqual(response.context['key'], 'value')
- self.assertIsInstance(response.context['view'], View)
+ request = RequestFactory().get('/dummy')
+ kwargs = {
+ 'foo': 'bar'
+ }
+ view = views.CustomTemplateView.as_instance(request, **kwargs)
@benoitbryon
benoitbryon Mar 3, 2014

A note about code length, because some users may say "using self.client.get() is more straightforward"...
We can also write the lines above (297-301) like this:

view = views.CustomTemplateView.as_instance(
    RequestFactory().get('/dummy'),
    foo='bar')

That said, the way they are written right now is fine too.

@benoitbryon benoitbryon commented on the diff Mar 3, 2014
tests/generic_views/urls.py
@@ -16,8 +16,6 @@
TemplateView.as_view()),
(r'^template/simple/(?P<foo>\w+)/$',
TemplateView.as_view(template_name='generic_views/about.html')),
- (r'^template/custom/(?P<foo>\w+)/$',
- views.CustomTemplateView.as_view(template_name='generic_views/about.html')),
@benoitbryon
benoitbryon Mar 3, 2014

This is one thing I like with this technique: we do not need to create and maintain URLconfs just for test purpose.

@benoitbryon

Is it possible to call as_instance() within as_view()?

@Keats

You mean MyView.as_view().as_instance() ? That won't work.

Do you have an example of a usecase for that ?

@benoitbryon

I mean replacing as_view.view implementation (https://github.com/Keats/django/blob/as_instance/django/views/generic/base.py#L63-L68) by a call to as_instance.
It may look like this:

    @classonlymethod
    def as_view(cls, **initkwargs):
        # ... (unchanged)

        def view(request, *args, **kwargs):
            self = cls(**initkwargs).as_instance(request, *args, **kwargs)
            return self.dispatch(request, *args, **kwargs)

        # ... (unchanged)

Note: the implementation above does not work in the current pull-request, because in the current pull-request as_instance() is a class-only method... but you get the idea.

@benoitbryon benoitbryon commented on an outdated diff Mar 5, 2014
django/views/generic/base.py
@@ -76,6 +76,19 @@ def view(request, *args, **kwargs):
update_wrapper(view, cls.dispatch, assigned=())
return view
+ @classonlymethod
@benoitbryon
benoitbryon Mar 5, 2014

Perhaps not suitable, as mentioned in #2368 (comment).

@benoitbryon benoitbryon commented on an outdated diff Mar 5, 2014
django/views/generic/base.py
@@ -76,6 +76,19 @@ def view(request, *args, **kwargs):
update_wrapper(view, cls.dispatch, assigned=())
return view
+ @classonlymethod
+ def as_instance(cls, request='', *args, **kwargs):
+ """
+ Creates an instance of the class, useful for isolated testing.
+ You can create a request through RequestFactory and pass args/kwargs as
+ you would do for a reverse() call.
+ """
+ view = cls()
+ view.request = request
@benoitbryon benoitbryon commented on an outdated diff Mar 5, 2014
django/views/generic/base.py
@@ -76,6 +76,19 @@ def view(request, *args, **kwargs):
update_wrapper(view, cls.dispatch, assigned=())
return view
+ @classonlymethod
+ def as_instance(cls, request='', *args, **kwargs):
+ """
+ Creates an instance of the class, useful for isolated testing.
+ You can create a request through RequestFactory and pass args/kwargs as
+ you would do for a reverse() call.
+ """
+ view = cls()
@benoitbryon
benoitbryon Mar 5, 2014

The **initkwargs used in as_view() is missing, isn't it?

@mjtamlyn
Django member

I quite like @benoitbryon's suggestion. Instead of implementing as_instance(), factor out as_view() to look like:

    @classonlymethod
    def as_view(cls, **initkwargs):
        # ... (unchanged)

        def view(request, *args, **kwargs):
            self = cls(**initkwargs).setup(request, *args, **kwargs)
            return self.dispatch(request, *args, **kwargs)

Then in your tests you can do cls(**initkwargs).setup(request, *args, **kwargs).

At the moment the current patch has a bit too much duplication for my liking.

@Keats

I updated the PR @benoitbryon @mjtamlyn
If you're ok with that way, I'll update the rest of the file

@mjtamlyn
Django member

Looks good to me. It will need documentation - in particular it might be worth documenting that you can pass extra arguments to the __init__ and they will get set on the view (if the view already has that attr), for example you can do:

view = MyDetailView(object=instance).setup(request, **kwargs)
view.get_context_data()

as SingleObjectMixin assumes that self.object is set before get_context_data().

@benoitbryon benoitbryon commented on an outdated diff Mar 12, 2014
django/views/generic/base.py
@@ -63,9 +63,7 @@ def view(request, *args, **kwargs):
self = cls(**initkwargs)
if hasattr(self, 'get') and not hasattr(self, 'head'):
self.head = self.get
@benoitbryon
benoitbryon Mar 12, 2014

Isn't it a part of the setup? What about moving it to setup() too?

@Keats Keats Fixed #20456: easier unit-testing for CBV
Added a class method to return an instance of a CBV.
This allows to unit test part of a CBV directly, without using
the client.
48f9e7a
@Keats

Started changing detail and list tests, will do edit/dates another day

@Keats

I did completely forget about that PR, let me know if the current changes are ok and I'll finish it

@timgraham
Django member

I think quite a bit more documentation is needed as Marc suggested. It would be nice to have a small commit that has the basic implementation with some tests and then make refactoring the existing tests a separate commit. See also our patch review checklist.

@mjtamlyn mjtamlyn self-assigned this Dec 24, 2014
@timgraham
Django member

Closing in absence of follow-up, feel free to send a new one if you want to continue working on this, thanks!

@timgraham timgraham closed this Mar 30, 2015
@grahamu grahamu referenced this pull request in revsys/django-test-plus Jun 23, 2015
Merged

Adding test methods and utilities for class-based views #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment