Added unit tests for djblets.auth.views.register

Review Request #10832 — Created Jan. 18, 2020 and submitted

Information

Djblets
release-1.0.x
617418a...

Reviewers

Added unit tests for djblets.auth.views.register

Changes for latest revision
Change 1:
Created new templates for testing: base.html and register.html.
This was to ensure that the render function within views.py could use a
template when running the test cases.

Change 2:
Added url to view mapping for djblets/auth/register so that the url could be
used for http requests (get/post) to test responses. In previous revisions, RequestFactory
was used, and the function register() from views.py was directly called within the test cases.

Change 3:
Used Django's test client instead of RequestFactory. This ensures that the register view actually
works as intended when its mapped url is called (rather than calling register() directly.

  • When register receives an http request that is not a POST request
  • When register receives a POST request, but an invalid form is passed
  • When register receives a POST request and a valid form is passed
Description From Last Updated

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (101 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

Docstring summaries must always begin on the """ line. If the docstring is single-line, the trailing """ should also be …

chipx86chipx86

The very first import on any module needs to be: from __future__ import unicode_literals This turns on Unicode strings on …

chipx86chipx86

Two important things: There must be only a single blank line between groups. Imports must always be alphabetical, so it's …

chipx86chipx86

Since we support Python 2.x, you'll need to always inherit from something. In this case, object.

chipx86chipx86

Same note as above. This is missing a period, and doesn't really tell us anything about the class.

chipx86chipx86

This is implied in a class, so you shouldn't need it.

chipx86chipx86

Same note about inheritence here.

chipx86chipx86

We have potentially thousands of unit tests running (in Review Board especially), so it's always important that our docstrings contain …

chipx86chipx86

The way we want to use this is to actually have SpyAgency mixed in to the parent class, like: class …

chipx86chipx86

We try to avoid using call_original=False where possible, because this masks other issues. Let's try without these and see if …

chipx86chipx86

It's generally best that we avoid calling views directly. Or mocking a request, for that matter. Instaed, we should take …

chipx86chipx86

You won't need this once you inherit from SpyAgency.

chipx86chipx86

No blank line here.

chipx86chipx86

W292 no newline at end of file

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

kpatenio
chipx86
  1. Thanks for the change!

    I have a handful of comments here, which really focuses on a few key areas:

    1. Getting things in shape for our documentation, coding, and unit test conventions.
    2. Best practices for working with kgb and spies.
    3. A better approach for how to test views.

    The end result will be a file that's probably half the size :) Come talk to me if you have any questions!

  2. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
    Show all issues

    Docstring summaries must always begin on the """ line. If the docstring is single-line, the trailing """ should also be on the same line.

    You'll want to go through our Writing Python Docs guide and make sure all your docstrings follow the standards we list there.

    1. Oh, and note that the unit test docstrings are special, because they're used for the output of unit tests, so they do have their own conventions, listed here.

  3. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
    Show all issues

    The very first import on any module needs to be:

    from __future__ import unicode_literals
    

    This turns on Unicode strings on Python 2.x, which ensures that 2.x and 3.x will work the same way.

    There must be a blank line between this and any other imports.

  4. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Two important things:

    1. There must be only a single blank line between groups.
    2. Imports must always be alphabetical, so it's clear where we should put new imports. This applies to the from ... part and to the list of things imported (import ...).
  5. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
    Show all issues

    Since we support Python 2.x, you'll need to always inherit from something. In this case, object.

  6. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
    Show all issues

    Same note as above. This is missing a period, and doesn't really tell us anything about the class.

  7. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
    Show all issues

    This is implied in a class, so you shouldn't need it.

  8. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
    Show all issues

    Same note about inheritence here.

  9. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
    Show all issues

    We have potentially thousands of unit tests running (in Review Board especially), so it's always important that our docstrings contain enough information to help us know what's being tested.

    As such, we want to list the class/function/whatever being tested in the docstring, like:

    """Testing register view when ..."""
    

    The test method must also contain information as well, so test_register_with_.... It must also be completely lowercase. So I'd call this test_register_without_post_method.

  10. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
    Show all issues

    The way we want to use this is to actually have SpyAgency mixed in to the parent class, like:

    class ViewTests(SpyAgency, TestCase):
    

    Then we'd call self.spy_on(...).

    This helps take care of some important cleanup tasks.

  11. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
    Show all issues

    We try to avoid using call_original=False where possible, because this masks other issues. Let's try without these and see if anything breaks. If something does, it means our test state isn't complete.

  12. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
    Show all issues

    It's generally best that we avoid calling views directly. Or mocking a request, for that matter. Instaed, we should take advantage of what Django offers, which is a nice test client that simulates HTTP requests. This ensures that we are working with real-world objects, testing real-world view/response paths.

    (Documentation on the test client)

    This is done by running code like:

    response = self.client.get('/some-url/')
    
    # or
    
    response = self.client.post(
        '/some-url/',
        {
            'key': 'value',
            ...
        })
    

    The latter for specifying form fields/POST data.

    You'll set up these URLs by defining a urls attribute to a module you'll need to create alongside this file. See the documentation.

    We also want to make sure we're always checking the responses, at a minimum checking the HTTP status code, but also checking whether it redirected to the right place (in this case).

  13. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
    Show all issues

    You won't need this once you inherit from SpyAgency.

  14. djblets/auth/tests/test_views.py (Diff revision 2)
     
     
     
     
    Show all issues

    No blank line here.

  15. 
      
anselina
  1. A few minor things on the review request itself:
    1. Can you add the bug ticket number (4588) to the "Bugs" field of this review request? It'll automatically link to the Splat ticket.
    2. Testing info should be listed in "Testing Done", not "Description".
    3. Can you wrap your "Description" and "Testing Done" to 80 chars to make it more readable?

  2. 
      
kpatenio
Review request changed
Change Summary:

Used test client instead of RequestFactory

Description:
~  

Added unit tests for djblets.auth.views.register. There's currently no test coverage for the register view; by adding unit tests, we can make sure that this doesn't break in the future. Tested the following cases:

  ~

Added unit tests for djblets.auth.views.register

-  
-  
  • When register receives an http request that is not a POST request
-  
  • When register receives a POST request, but an invalid form is passed
-  
  • When register receives a POST request and a valid form is passed
Testing Done:
  +
  • When register receives an http request that is not a POST request
  +
  • When register receives a POST request, but an invalid form is passed
  +
  • When register receives a POST request and a valid form is passed
Bugs:
Commit:
3310c96d6f8d4ac3187ab69374c1317f5deb84fc
1de1e18ac48d23894b3b6d8e7f94fdfdcd99732c

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
kpatenio
hxqlin
  1. 
      
  2. Where do the variable values come from? ie. title, form? Are they populated as a result of the request?

    1. Hi Hannah! Thanks for reviewing my code. I looked into this and found a couple resources that might help:

      • Django template language: https://docs.djangoproject.com/en/3.0/ref/templates/language/#the-django-template-language
      • built-in template tags/filters: https://docs.djangoproject.com/en/3.0/ref/templates/builtins/#built-in-template-tags-and-filters
      • forms and templates: https://docs.djangoproject.com/en/3.0/topics/forms/#the-template

      From what I've read from Django's docs, this is what I understand: * title is just the name of the block on line 3. register.html's parent, base.html defines a block named
      title within the <title/> html tag. So, on line 3 for register.html', we're overwriting the title block that
      was originally defined in the parent in order to add the title "Register an account".

      • content is also just the name of the block. It references + overwrites base.html's content block,
        which is within a <body/> tag. form comes from Django's template language (see the third resource above).
        The form will come from a request (likely a POST request) and we'll load it as the main content of our page.

      Might be easier to understand after seeing register.html's parent page, base.html:

      <html>
       <head>
        <title>{% block title %} {% endblock  %}</title>
       </head>
       <body>
      {% block content %}{% endblock %}
       </body>
      </html>
      

      I hope this helps! Let me know if that's still unclear or you have more questions. :)

  3. djblets/auth/tests/test_views.py (Diff revision 4)
     
     
     
     
     
     
     

    Do these functions run before and after every test?

    1. I believe they do. Djblet's setUp and tearDown functions are based on Python unittest's functions (of the same name).

      This link helped me understand what they both do (from the unittest perspective though): https://stackoverflow.com/a/6854727

  4. 
      
xiaole2
  1. 
      
  2. djblets/auth/tests/test_views.py (Diff revision 4)
     
     
     
     
     
     
     

    @Hannah& Kat

    Yes, they do.

    1. setUp() and tearDown() are optional functions. We can use them to factor out the code common to all tests.
    2. setUp() runs before each test in the class. tearDown() runs after each test in the class.
    3. Testcases are often used in cooperative multiple inheritance, so we should call super in these methods so the base class's setUp and tearDown methods also get called.

    references:
    1.(https://riptutorial.com/python/example/13280/test-setup-and-teardown-within-a-unittest-testcase)

  3. 
      
david
  1. Going to make some minor tweaks to this and land it. Thanks!

  2. 
      
kpatenio
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (de21e3d)