Clean up and consolidate avatar unit tests

Review Request #8528 — Created Nov. 7, 2016 and submitted

Information

Review Board
release-3.0.x
221c6b9...

Reviewers

The avatar services tests had a few issues, namely:

  • not resetting the registry after test completion;
  • templatetag tests being spread out over two files;
  • some tests not being updated for the new avatar services API; and
  • using improper methods to generate dummy requests.

Some of these issues were causing test failures, while others were more
stylistic issues. All of these issues have been fixed and the avatar
unit tests now all pass.

Ran unit tests.

Description From Last Updated

'User' imported but unused

reviewbotreviewbot

'HttpRequest' imported but unused

reviewbotreviewbot

'Profile' imported but unused

reviewbotreviewbot

'avatar_services' imported but unused

reviewbotreviewbot

Can you put all of this into parens?

daviddavid

Here too?

daviddavid

And here?

daviddavid

And here?

daviddavid

Alphabetical order.

chipx86chipx86

Doesn't this leak state that could theoretically impact other tests? Can we instead save/restore values? Also, it looks like we're …

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/avatars/tests.py
        reviewboard/avatars/templatetags/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/avatars/tests.py
        reviewboard/avatars/templatetags/tests.py
    
    
  2. reviewboard/avatars/tests.py (Diff revision 1)
     
     
     'User' imported but unused
    
  3. reviewboard/avatars/tests.py (Diff revision 1)
     
     
     'HttpRequest' imported but unused
    
  4. reviewboard/avatars/tests.py (Diff revision 1)
     
     
     'Profile' imported but unused
    
  5. reviewboard/avatars/tests.py (Diff revision 1)
     
     
     'avatar_services' imported but unused
    
  6. 
      
chipx86
  1. I think we should have a reviewboard/avatars/tests/, and have test_templatetags.py inside of that, instead of having tests.py in reviewboard/avatars/templatetags/.

    The reason is that, as test suites grow, we tend to split things out into a tests/ directory within the app directory, and all unit tests in there. If we were to do that, but template tag tests lived inside the templatetags/ directory, it wouldn't be run as part of the test suite.

    It also avoids importing tests.py in production, which Django will do when looking for template registries.

  2. 
      
brennie
david
  1. 
      
  2. reviewboard/avatars/tests.py (Diff revision 2)
     
     
     
     
     
     
     

    Can you put all of this into parens?

  3. reviewboard/avatars/tests.py (Diff revision 2)
     
     
     
     
     
     

    Here too?

  4. reviewboard/avatars/tests.py (Diff revision 2)
     
     
     
     
     
     
     

    And here?

  5. reviewboard/avatars/tests.py (Diff revision 2)
     
     
     
     
     

    And here?

  6. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/avatars/tests.py (Diff revision 3)
     
     
     
     

    Alphabetical order.

  3. reviewboard/avatars/tests.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     

    Doesn't this leak state that could theoretically impact other tests? Can we instead save/restore values?

    Also, it looks like we're enabling gravatars twice.

    1. Oops, we don't need to reset them on test tear down because we inherit from the AvatarServicesTestMixin

  4. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (b682119)
Loading...