Sandbox reviewboard.attachments.mimetypes.MimetypeHandler

Review Request #6532 — Created Oct. 29, 2014 and submitted

justy777
Review Board
master
aac3e3d...
reviewboard, students

Extensions that create a MimetypeHandler subclass can throw exceptions inside Review Board. The sections of Review Board that call those methods have been sandboxed.

Now when a MimetypeHandler subclass from an extension throws an exception, and Review Board logs the errors. The error logs give enough information to find which method in the MimetypeHandler subclass threw the exception.

Unit tests have been written to make sure that functions from an MimetypeHandler 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

"... for MimetypeHandler %r: ...' Same below.

chipx86chipx86

This isn't testing extension sandboxing. Missing a trailing period.

chipx86chipx86

On this test and the others, we should also ensure the function we're expecting to call is in fact called ...

chipx86chipx86

No need for the _mimetypehandler on these.

chipx86chipx86

We shouldn't be accessing private functions. Instead, access the property.

chipx86chipx86

As above, we should be using the property, not the private function.

chipx86chipx86

These are only testing our code and not anything with a custom MimetypeHandler. Not necessary to do that. Also, there's ...

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'register_mimetype_handler' imported but unused

reviewbotreviewbot

'unregister_mimetype_handler' imported but unused

reviewbotreviewbot

'FileAttachmentThumbnailHook' imported but unused

reviewbotreviewbot

undefined name 'MimetypeHandler'

reviewbotreviewbot

'SpyAgency' imported but unused

reviewbotreviewbot

I believe you'll want to tear down both parent classes explicitly: SpyAgency.tearDown(self) BaseFileAttachmentTestCase.tearDown(self)

daviddavid

Instead of creating a filediff and using create_from_filediff, you can just call self.create_file_attachment(). That way you can probably also avoid ...

daviddavid

This still needs to return something (None).

chipx86chipx86

This still needs to return something (None).

chipx86chipx86

I prefer these to be in the form of: """Tested FileAttachment sandboxes MimetypeHandler.get_thumbnail""" And similar. The reason being that these ...

chipx86chipx86

This should be "Testing" (not "Tested")

daviddavid

And here.

daviddavid

And here.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. reviewboard/extensions/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/models.py (Diff revision 2)
     
     
     

    "... for MimetypeHandler %r: ...'

    Same below.

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

    This isn't testing extension sandboxing.

    Missing a trailing period.

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

    On this test and the others, we should also ensure the function we're expecting to call is in fact called by using a spy (and making sure to set it to call the original function).

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

    No need for the _mimetypehandler on these.

  6. reviewboard/attachments/tests.py (Diff revision 2)
     
     

    We shouldn't be accessing private functions. Instead, access the property.

  7. reviewboard/attachments/tests.py (Diff revision 2)
     
     

    As above, we should be using the property, not the private function.

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

    These are only testing our code and not anything with a custom MimetypeHandler. Not necessary to do that. Also, there's no sandboxing there anyway.

  9. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 3)
     
     
     'register_mimetype_handler' imported but unused
    
  3. reviewboard/extensions/tests.py (Diff revision 3)
     
     
     'unregister_mimetype_handler' imported but unused
    
  4. reviewboard/extensions/tests.py (Diff revision 3)
     
     
     'FileAttachmentThumbnailHook' imported but unused
    
  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 4)
     
     
     undefined name 'MimetypeHandler'
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/extensions/tests.py
        reviewboard/attachments/models.py
    
    
  2. reviewboard/extensions/tests.py (Diff revision 5)
     
     
     'SpyAgency' imported but unused
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/attachments/tests.py (Diff revision 6)
     
     

    I believe you'll want to tear down both parent classes explicitly:

    SpyAgency.tearDown(self)
    BaseFileAttachmentTestCase.tearDown(self)
    
    1. It's valid to use super() here. We do it elsewhere, and I just tested that all tearDown methods are properly called with super().

      I had him switch to using super() in a previous change. Poor guy is going back and forth :)

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
justy777
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/attachments/tests.py (Diff revision 8)
     
     
     

    Instead of creating a filediff and using create_from_filediff, you can just call self.create_file_attachment(). That way you can probably also avoid using the test_scmtools fixtures, which will speed up execution of these tests a bit.

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/models.py (Diff revision 9)
     
     
     
     
     

    This still needs to return something (None).

  3. reviewboard/attachments/models.py (Diff revision 9)
     
     
     
     
     

    This still needs to return something (None).

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

    I prefer these to be in the form of:

    """Tested FileAttachment sandboxes MimetypeHandler.get_thumbnail"""
    

    And similar.

    The reason being that these functions (get_thumbnail, etc.) are not themselves sandboxing anything. FileAttachment is the thing doing the sandboxing.

  5. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/tests.py
        reviewboard/attachments/models.py
    
    
  2. 
      
justy777
david
  1. In your description, there are a couple changes I want to make. First, it should always be "Review Board" (not "Reviewboard"). Second, your second paragraph in the description is full of fragments and an incorrect use of a semicolon. A basic fix for this would be to change the semicolon to a comma and combine it into one sentence. Nicer might be to use shorter sentences but be a bit more specific about what happens.

  2. reviewboard/attachments/tests.py (Diff revision 11)
     
     

    This should be "Testing" (not "Tested")

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

    And here.

  4. reviewboard/attachments/tests.py (Diff revision 11)
     
     

    And here.

  5. 
      
justy777
justy777
david
  1. Ship It!
  2. 
      
justy777
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (8635ce4)
Loading...