Sandbox reviewboard.accounts.AuthBackend

Review Request #6399 — Created Oct. 4, 2014 and submitted

justy777
Review Board
master
6559
bc8a283...
reviewboard, students

Extensions that create an AuthBackend subclass (using the AuthBackendHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

Now when an AuthBackend subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the AuthBackend subclass threw the exception.

Unit tests have been written to make sure that functions from an AuthBackend subclass have been called, and when an exception is thrown it gets logged.

The tests fail without the sanboxing, and succeed with it.

Description From Last Updated

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (105 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (111 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 31 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Can you include the backend name in here? Right now it's not possible to know which backend is failing from ...

daviddavid

Here too?

daviddavid

Here too?

daviddavid

Here too?

daviddavid

There should also be a blank line here. Undo this.

brenniebrennie

There should be 2 blank lines between classes. Undo this.

brenniebrennie

You can skip the try/catch and just call form.clean_old_password(). If an exception is raised and not caught in a test ...

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Can you include the backend name in here?

daviddavid

Can you include the backend name in here?

daviddavid

And here.

daviddavid

And here.

daviddavid

Col: 28 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E113 unexpected indentation

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 52 W291 trailing whitespace

reviewbotreviewbot

Col: 28 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Auth backends have a user-visible name, so you can use backend.name instead of backend.__class__.name. Here and all the other logging ...

daviddavid

There shouldn't be a blank line here (django, djblets, and kgb are all "third party" packages)

daviddavid

What's the point of this?

daviddavid

L before R

daviddavid

Use # for comments. """ is a docstring.

daviddavid

This should be commented too.

daviddavid

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Add a trailing comma here.

daviddavid

Add a trailing comma here.

daviddavid

No # at the end.

daviddavid

Add a trailing comma here.

daviddavid

No # at the end.

daviddavid

Add a trailing comma here.

daviddavid

No # at the end.

daviddavid

This will catch the ValidationError that's raised inside the try.

daviddavid

Blank line between these.

chipx86chipx86

The logging errors should use %r with backend, so that the log message will include the specific class and any ...

chipx86chipx86

These are human-readable strings. They need to be proper sentences, and should be targeted to the end user. Unlike with ...

chipx86chipx86

Blank line. Same below.

chipx86chipx86

Blank line.

chipx86chipx86

No need for Class, and Test is too generic. Should be something like SandboxAuthBackend.

chipx86chipx86

No point in storing this in a variable.

chipx86chipx86

This should be something like: self.assertEqual(e.message, 'blah blah') If the string still wraps, this may be better: self.assertEqual( e.message, 'blah ...

chipx86chipx86

It's unclear how this relates to the function being tested. Looking through the change, I realize they're not. Instead, these ...

chipx86chipx86

blank line before this.

chipx86chipx86

Use %r, and start the arguments on a new line.

chipx86chipx86

Use %r, and start the arguments on a new line.

chipx86chipx86

Use %r, and start the arguments on a new line.

chipx86chipx86

Use %r, and start the arguments on a new line.

chipx86chipx86

There's no need for this, since we'll end up with either a value or an exception in the next block.

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

Similar to my comments in the columns change, you don't actually need extensions for these tests. You only need extensions ...

chipx86chipx86

There's nothing here to test for breakages, since these functions don't raise any exceptions. On all these changes, you'll need ...

chipx86chipx86

Two blank lines at the top level between classes.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

This isn't actually testing anything.

chipx86chipx86

Again, extensions are useless here.

chipx86chipx86

Extensions are useless here too.

chipx86chipx86

If we're not testing this function, we should remove it. Otherwise, it should throw something. Same in all other cases ...

chipx86chipx86

Same comments.

chipx86chipx86

Col: 43 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 43 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Should have a trailing period.

chipx86chipx86

Should just be: super(SandboxTests, self).tearDown()

chipx86chipx86

We have no verification that authenticate() is ever called. We should use spy_on() and check that it was called with ...

chipx86chipx86

These docstrings should be in this format: """Testing AuthBackend sandboxing for <func>"""

chipx86chipx86

For this, you can use: self.assertRaisesMessage( ValidationError, "<message here>", lambda: form.clean_old_password())

chipx86chipx86

We have no verification that update_password() is ever called. We should use spy_on() and check that it was called with ...

chipx86chipx86

We have no verification that update_name() is ever called. We should use spy_on() and check that it was called with ...

chipx86chipx86

We do this in all the tests, so it can move into setUp().

chipx86chipx86

We do this a lot, so it can probably just move into setUp().

chipx86chipx86

We have no verification that update_email() is ever called. We should use spy_on() and check that it was called with ...

chipx86chipx86

This should be using super(...) still. Why was it changed?

chipx86chipx86

This should say: """Testing AuthBackendHook sandboxes AuthBackend registration""" Note the "sandboxes" part, which will help give a clue if tests ...

chipx86chipx86

For API resource unit tests, we don't want to introduce any new classes. Instead, the test should be part of ...

chipx86chipx86

This docstring isn't correct, and doesn't match the rest of the docstrings in this class. It should describe the URL, ...

chipx86chipx86

This should be treated as a failure, not success.

daviddavid

I mentioned this in the other review request, but this isn't correct.

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
AD
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 1)
     
     

    Is it possible for get_or_create(username) to fail?

    1. The whole point is to get it to throw an exception.

  3. reviewboard/extensions/tests.py (Diff revision 1)
     
     

    Is it possible for get_or_create(username) to fail?

  4. 
      
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  4. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  8. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (111 > 79 characters)
    
  9. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  10. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  11. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  12. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  13. Col: 80
     E501 line too long (82 > 79 characters)
    
  14. reviewboard/webapi/resources/user.py (Diff revision 2)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  15. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. reviewboard/extensions/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  4. reviewboard/webapi/resources/user.py (Diff revision 3)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  5. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. reviewboard/extensions/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  4. reviewboard/webapi/resources/user.py (Diff revision 4)
     
     
    Col: 31
     E128 continuation line under-indented for visual indent
    
  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 5)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 6)
     
     

    There should also be a blank line here. Undo this.

  3. reviewboard/extensions/tests.py (Diff revision 6)
     
     

    There should be 2 blank lines between classes. Undo this.

  4. 
      
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
     

    Can you include the backend name in here? Right now it's not possible to know which backend is failing from the logged messages.

  3. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
     

    Here too?

  4. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
     

    Here too?

  5. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
     

    Here too?

  6. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     
     
     

    You can skip the try/catch and just call form.clean_old_password(). If an exception is raised and not caught in a test case, the test will fail.

  7. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  8. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  9. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  10. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  11. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  12. reviewboard/extensions/tests.py (Diff revision 6)
     
     
     

    Same here.

  13. Can you include the backend name in here?

  14. Can you include the backend name in here?

  15. reviewboard/webapi/resources/user.py (Diff revision 6)
     
     
     

    And here.

  16. reviewboard/webapi/resources/user.py (Diff revision 6)
     
     
     

    And here.

  17. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
    Col: 28
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 7)
     
     
    Col: 55
     W291 trailing whitespace
    
  4. reviewboard/extensions/tests.py (Diff revision 7)
     
     
    Col: 13
     E113 unexpected indentation
    
  5. Col: 80
     E501 line too long (80 > 79 characters)
    
  6. reviewboard/webapi/resources/user.py (Diff revision 7)
     
     
    Col: 52
     W291 trailing whitespace
    
  7. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/accounts/forms/pages.py (Diff revision 8)
     
     
    Col: 28
     E127 continuation line over-indented for visual indent
    
  3. Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 9)
     
     

    Auth backends have a user-visible name, so you can use backend.name instead of backend.__class__.name. Here and all the other logging calls.

  3. reviewboard/extensions/tests.py (Diff revision 9)
     
     

    There shouldn't be a blank line here (django, djblets, and kgb are all "third party" packages)

  4. reviewboard/extensions/tests.py (Diff revision 9)
     
     
     

    What's the point of this?

    1. It spoofs a function call to messages.add_message() at the end of form.save(), that would otherwise raise a MessageFailure Exception because I have not 'installed' something in Middleware.

      raise MessageFailure('You cannot add messages without installing '
      MessageFailure: You cannot add messages without installing django.contrib.messages.middleware.MessageMiddleware

    2. Would you mind adding a comment above the places where you do that explaining this?

    3. All the spy_on()'s or just the two that supress other Exceptions?

    4. The ones where you're specifically spying on messages.add_message. The other ones are pretty self-explanatory.

  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/extensions/tests.py (Diff revision 11)
     
     

    Use # for comments. """ is a docstring.

  3. reviewboard/extensions/tests.py (Diff revision 11)
     
     

    This should be commented too.

  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 12)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  3. reviewboard/extensions/tests.py (Diff revision 12)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  4. reviewboard/extensions/tests.py (Diff revision 12)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 13)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  3. reviewboard/extensions/tests.py (Diff revision 13)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  4. reviewboard/extensions/tests.py (Diff revision 13)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
david
  1. Just a few more trivial style things.

  2. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    Add a trailing comma here.

  3. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    Add a trailing comma here.

  4. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    No # at the end.

  5. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    Add a trailing comma here.

  6. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    No # at the end.

  7. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    Add a trailing comma here.

  8. reviewboard/extensions/tests.py (Diff revision 14)
     
     

    No # at the end.

  9. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
david
  1. When I run the unit tests with this patch applied I get a lot of failures.

  2. reviewboard/accounts/forms/pages.py (Diff revision 15)
     
     

    This will catch the ValidationError that's raised inside the try.

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/extensions/tests.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
chipx86
  1. Before reading through these comments, go read through the sandboxing for datagrid columns. My comment is going to be based on that discussion.

    Similar to datagrid column testing, you have a bunch of tests under the "Extensions" umbrella that have nothing to do with extensions.

    If a particular part of the codebase is being modified to not break when calling a particular thing, then the unit test belongs alongside the other unit tests for that part of the codebase. So for instance, an API resource calls a function and needs to handle breakages in that function. That API resource's test suite should be updated with a test that handles that case.

    The tests that belong with the extension are:

    1) Does enabling an extension and instantiating the hook result in the auth backend properly being registered?
    2) Does disabling the extension result in the auth backend properly being unregistered?

    The rest of the tests are testing specific instances where functions are used (like in forms). Those tests belong with the tests for those instances (like accounts/tests.py).

  2. reviewboard/accounts/forms/pages.py (Diff revision 18)
     
     
     

    Blank line between these.

  3. reviewboard/accounts/forms/pages.py (Diff revision 18)
     
     
     

    The logging errors should use %r with backend, so that the log message will include the specific class and any additional information the author of the class provides for the class-to-string representation.

    Same below.

  4. reviewboard/accounts/forms/pages.py (Diff revision 18)
     
     
     

    These are human-readable strings. They need to be proper sentences, and should be targeted to the end user. Unlike with the log message (which is there for diagnosing problems), including extra information in a validation message doesn't do the end user much good, as they likely can't do a thing about it.

    How about something like:

    Unexpected error when validating the password. Please contact the administrator.
    
  5. reviewboard/accounts/forms/pages.py (Diff revision 18)
     
     
     

    Blank line.

    Same below.

  6. reviewboard/accounts/forms/pages.py (Diff revision 18)
     
     
     

    Blank line.

  7. reviewboard/extensions/tests.py (Diff revision 18)
     
     

    No need for Class, and Test is too generic. Should be something like SandboxAuthBackend.

  8. reviewboard/extensions/tests.py (Diff revision 18)
     
     

    No point in storing this in a variable.

  9. reviewboard/extensions/tests.py (Diff revision 18)
     
     
     

    This should be something like:

    self.assertEqual(e.message,
                     'blah blah')
    

    If the string still wraps, this may be better:

    self.assertEqual(
        e.message,
        'blah blah ...')
    
  10. reviewboard/extensions/tests.py (Diff revision 18)
     
     
     
     
     
     
     

    It's unclear how this relates to the function being tested.

    Looking through the change, I realize they're not. Instead, these are two cases that happen to call that function. You wouldn't want to include every single call site ever in this place.

    Instead, what you want is to have unit tests in the resource's own test file that covers the case where the function fails.

  11. reviewboard/extensions/tests.py (Diff revision 18)
     
     

    blank line before this.

  12. Use %r, and start the arguments on a new line.

  13. Use %r, and start the arguments on a new line.

  14. reviewboard/webapi/resources/user.py (Diff revision 18)
     
     
     

    Use %r, and start the arguments on a new line.

  15. reviewboard/webapi/resources/user.py (Diff revision 18)
     
     
     
     

    Use %r, and start the arguments on a new line.

  16. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 19)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 19)
     
     

    There's no need for this, since we'll end up with either a value or an exception in the next block.

  3. reviewboard/accounts/forms/pages.py (Diff revision 19)
     
     
     

    Blank line between these.

  4. reviewboard/accounts/forms/pages.py (Diff revision 19)
     
     
     

    Blank line between these.

  5. reviewboard/accounts/tests.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     

    Similar to my comments in the columns change, you don't actually need extensions for these tests. You only need extensions for the ones testing extension-specific functionality (like the hooks).

  6. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     
     
     
     
     

    There's nothing here to test for breakages, since these functions don't raise any exceptions.

    On all these changes, you'll need to make sure that every one of these tests fail without your sandboxing, and passes with them.

  7. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     
     

    Two blank lines at the top level between classes.

  8. reviewboard/extensions/tests.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     

    This isn't actually testing anything.

  9. reviewboard/webapi/tests/test_review_request.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     

    Again, extensions are useless here.

  10. reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     

    Extensions are useless here too.

  11. If we're not testing this function, we should remove it. Otherwise, it should throw something.

    Same in all other cases where you have a custom auth backend for testing.

  12. reviewboard/webapi/tests/test_user.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same comments.

  13. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 43
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/extensions/tests.py (Diff revision 20)
     
     
    Col: 43
     E128 continuation line under-indented for visual indent
    
  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/tests.py (Diff revision 23)
     
     

    Should have a trailing period.

  3. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     
     

    Should just be:

    super(SandboxTests, self).tearDown()
    
  4. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     

    We have no verification that authenticate() is ever called. We should use spy_on() and check that it was called with the expected parameters.

  5. reviewboard/accounts/tests.py (Diff revision 23)
     
     

    These docstrings should be in this format:

    """Testing AuthBackend sandboxing for <func>"""
    
  6. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     
     
     
     
     

    For this, you can use:

    self.assertRaisesMessage(
        ValidationError,
        "<message here>",
        lambda: form.clean_old_password())
    
  7. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     

    We have no verification that update_password() is ever called. We should use spy_on() and check that it was called with the expected parameter.

  8. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     

    We have no verification that update_name() is ever called. We should use spy_on() and check that it was called with the expected parameter.

  9. reviewboard/accounts/tests.py (Diff revision 23)
     
     

    We do this in all the tests, so it can move into setUp().

  10. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     
     

    We do this a lot, so it can probably just move into setUp().

  11. reviewboard/accounts/tests.py (Diff revision 23)
     
     
     

    We have no verification that update_email() is ever called. We should use spy_on() and check that it was called with the expected parameter.

  12. reviewboard/extensions/tests.py (Diff revision 23)
     
     
     

    This should be using super(...) still. Why was it changed?

  13. reviewboard/extensions/tests.py (Diff revision 23)
     
     

    This should say:

    """Testing AuthBackendHook sandboxes AuthBackend registration"""
    

    Note the "sandboxes" part, which will help give a clue if tests ever fail.

    Same below with unregistration.

  14. reviewboard/webapi/tests/test_review_request.py (Diff revision 23)
     
     
     
     

    For API resource unit tests, we don't want to introduce any new classes. Instead, the test should be part of the existing class, and should go through an API call.

    That is, we should set up a failure case, perform an API call, and make sure we get back a sane response and not a 500. (Also need to test that it does generate a 500 without the sandboxing.)

    This applies to the other API tests as well.

    1. Note that this comment applies to the draft resource too.

  15. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/extensions/tests.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/tests/test_review_request.py (Diff revisions 25 - 26)
     
     

    This docstring isn't correct, and doesn't match the rest of the docstrings in this class. It should describe the URL, parameters, and condition, like:

    """Testing ... API with submit_as= and AuthBackend.get_or_create_user failure"""
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 27)
     
     

    This should be treated as a failure, not success.

  3. I mentioned this in the other review request, but this isn't correct.

    1. NOTE: This fix depends on my bug fix https://reviews.reviewboard.org/r/6559/, and will get fixed as soon as my bug fix is accepted.

  4. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/resources/review_request.py
        reviewboard/accounts/forms/pages.py
        reviewboard/webapi/tests/test_review_request.py
        reviewboard/accounts/tests.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/webapi/tests/test_user.py
    
    
  2. 
      
justy777
david
  1. Ship It!

  2. 
      
justy777
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (7bb05bc)
Loading...