• 
      

    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)