Default Action Tests for Read-Only Mode

Review Request #8861 — Created March 31, 2017 and submitted

Information

khp
Review Board
master

Reviewers

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

Description From Last Updated

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

szhangszhang

Do we want to refactor this into a mixin that does common tests for actions that respect read-only mode?

brenniebrennie

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

szhangszhang

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

szhangszhang

Closing quote on next line.

brenniebrennie

LIkewise.

brenniebrennie

I think you should be more specific with the condition for the unit test here. So, you should include a …

szhangszhang

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

szhangszhang

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 …

brenniebrennie

Same here (mention the permission?).

szhangszhang

See above comment about user fixture.

brenniebrennie

This probably does not really matter, but I think in the previous code you put 'perms' before 'request'. Probably should …

JO joycezhou47

Same here (mention the permission?).

szhangszhang

Same here (mention the permission?).

szhangszhang

Closing quote on next line.

brenniebrennie

We are trying to move away from Mock (as it is a rather heavy dependency), although I understand that previous …

brenniebrennie

Missing a period.

brenniebrennie

Closing quote on next line.

brenniebrennie

See my previous comment about the user fixture.

brenniebrennie

See my previous comment about the user fixture. You can use the admin user here.

brenniebrennie

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

brenniebrennie

Two blank line between imports and first declaration.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

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

brenniebrennie

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 …

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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 …

brenniebrennie
KH
szhang
  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. 
      
brennie
  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. 
      
JO
  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. 
      
KH
brennie
  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. 
      
KH
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (768aab467)

Loading...