Default Action Tests for Read-Only Mode

Review Request #8861 - Created March 31, 2017 and updated

Kanghee Park
Review Board
master
8810, 8811, 8657, 8812, 8824, 8847, 8648
reviewboard, students

Read-only mode is a setting an admin can enable to prevent writes to
the database. This can be used when the site is under maintenence or
being upgraded. This commit adds testing for changes to the action
buttons visible in the tabs of a review request.

Run pytests - all pass except some known errors

  • 1
  • 0
  • 27
  • 1
  • 29
Description From Last Updated
When you do: class Foo: @add_fixtures(['..']) def test_foo(self): pass it is actually equivalent to: class Foo: def test_foo(self): pass test_foo ... Barret Rennie Barret Rennie
Kanghee Park
Simon Zhang
  1. Whoa, this is a long diff. Here some little things I noticed:

  2. Can you move this to the top of the imports, since mock is part of the Python Standard Library?

    And a blank line in-between that and the django/djblets imports, of course.

    1. mock is not part of the standard library.

  3. You didn't make these changes, but shouldn't the docstrings for the tests involving anonymous users says ...with anonymous user instead of ...with authenticated user?

    Same below.

  4. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     
     

    I noticed in the tests above that they specify that the user is authenticated. Perhaps you could mention that as well?

    Same with the test directly below.

  5. I think you should be more specific with the condition for the unit test here.

    So, you should include a mention about the can_change_status permission. Something like Testing CloseMenuAction.should_render for user with can_change_status permission but in read-only mode

    Same with some tests below.

  6. I think you forgot to add periods at the end of your comments with these changes.

    Same with the other comments in your changes.

  7. Same here (mention the permission?).

  8. Same here (mention the permission?).

  9. Same here (mention the permission?).

  10. 
      
Barret Rennie
  1. 
      
  2. Do we want to refactor this into a mixin that does common tests for actions that respect read-only mode?

  3. Closing quote on next line.

  4. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     

    We have test users from the test-users fixture. You can do:

    self.request.user = User.objects.get(username='doc')
    

    This user is not a superuser (username admin is).

  5. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     

    See above comment about user fixture.

  6. Closing quote on next line.

  7. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     

    We are trying to move away from Mock (as it is a rather heavy dependency), although I understand that previous tests in this file are using it.

    Ideally all of these would be replaced with a fake resolver match class:

    class FakeResolverMatch(object):
        def __init__(self, url_name):
            self.url_name = url_name
    

    This will ultimately result in fewer lines of code.

  8. Missing a period.

  9. Closing quote on next line.

  10. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     

    See my previous comment about the user fixture.

  11. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     

    See my previous comment about the user fixture.

    You can use the admin user here.

  12. reviewboard/reviews/tests/test_actions.py (Diff revision 2)
     
     
     
     
     

    We have an admin user:

    admin = User.objects.get(username='admin')
    review_request = self.create_review_request(
        create_repository=True,
        submitter=admin)
    self.request.user = admin
    
  13. 
      
Joyce Zhou
  1. 
      
  2. This probably does not really matter, but I think in the previous code you put 'perms' before 'request'. Probably should keep this consistent?

  3. 
      
Kanghee Park
Barret Rennie
  1. 
      
  2. reviewboard/reviews/tests/test_actions.py (Diff revision 3)
     
     
     

    Two blank line between imports and first declaration.

  3. Missing docstring.

  4. Missing docstring.

  5. This should indicate in its name that its a mixin of tests, e.g., ReadOnlyActionTestsMixin.

    Missing docstring.

  6. reviewboard/reviews/tests/test_actions.py (Diff revision 3)
     
     
     
     
     

    When you do:

    class Foo:
        @add_fixtures(['..'])
        def test_foo(self): pass
    

    it is actually equivalent to:

    class Foo:
        def test_foo(self): pass
        test_foo = add_fixtures(['..'])(test_foo)
    

    so you should actually replace the functions on the instance:

    self.test_should_render_with_user_in_read_only = _add_fixtures(self.test_should_render_with_user_in_read_only)
    self.test_should_render_with_superuser_in_read_only = _add_fixtures(self.test_should_render_with_superuser_in_read_only)
    
    1. Hey Barret

      I've tried a bunch of different stuff here but not completely sure about the reasoning for why I have to do it this way, but I think it's something like this:

      The decorator function, when used as a decorator (i.e. @add_fixtures([ ... ]) is given the function, not the wrapped bound or unbound method. So I get attribute errors from the decorator trying to do func._fixtures = ... if a method is passed in, as _add_fixtures(self.test_should_render_with_user_in_read_only) would do, but no errors if used as a regular decorator. This explains why I'm using .__func__, to access the function in the wrapped method.

      I tried setting self.test_should_render_with_user_in_read_only = ..., but if this is done in __init__, it seems to replace the current bounded method with the function, and I lose the reference to self, as it now expects the self argument.

      It seems a little hacky, but I wasn't sure of a better way to both pass fixtures into the mixin from the superclass and add them to the test functions, as I can't add arguments to the decorators like usual (@add_fixtures(...)) if I want to use a mixin.

  7. reviewboard/reviews/tests/test_actions.py (Diff revision 3)
     
     
     
     
     
     
     
     

    Instead of doing this every test, how about...

  8. Use ~reviewboard.reviews.actions.BaseReviewRequestAction and it will render the link with the text of only "BaseReviewRequestAction".

  9. reviewboard/reviews/tests/test_actions.py (Diff revision 3)
     
     
     
     
     
     
     
     

    we do:

    try:
        self.siteconfig.set('site_read_only', True)
        self.siteconfig.save()
    
        #... rest of test
    finally:
        self.siteconfig.set('site_read_only', defaults.get('site_read_only'))
        self.siteconfig.save()
    

    This prevents some unnecessary siteconfig saving when nothing has changed.

    Same for the below test.

    1. Nice catch, this is much better.

  10. 
      
Kanghee Park
Review request changed

Checks run (2 succeeded, 1 failed with error)

JSHint passed.
PEP8 Style Checker internal error.
Pyflakes passed.
Loading...