Sandbox reviewboard.attachments.mimetypes.MimetypeHandler
Review Request #6532 — Created Oct. 29, 2014 and submitted
| Information | |
|---|---|
| justy777 | |
| Review Board | |
| master | |
| aac3e3d... | |
| Reviewers | |
| 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. |
 chipx86
|
|
|
This isn't testing extension sandboxing. Missing a trailing period. |
chipx86
|
|
|
On this test and the others, we should also ensure the function we're expecting to call is in fact called … |
chipx86
|
|
|
No need for the _mimetypehandler on these. |
chipx86
|
|
|
We shouldn't be accessing private functions. Instead, access the property. |
chipx86
|
|
|
As above, we should be using the property, not the private function. |
chipx86
|
|
|
These are only testing our code and not anything with a custom MimetypeHandler. Not necessary to do that. Also, there's … |
chipx86
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
 reviewbot
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
'register_mimetype_handler' imported but unused |
reviewbot
|
|
|
'unregister_mimetype_handler' imported but unused |
reviewbot
|
|
|
'FileAttachmentThumbnailHook' imported but unused |
reviewbot
|
|
|
undefined name 'MimetypeHandler' |
reviewbot
|
|
|
'SpyAgency' imported but unused |
reviewbot
|
|
|
I believe you'll want to tear down both parent classes explicitly: SpyAgency.tearDown(self) BaseFileAttachmentTestCase.tearDown(self) |
 david
|
|
|
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 … |
david
|
|
|
This still needs to return something (None). |
chipx86
|
|
|
This still needs to return something (None). |
chipx86
|
|
|
I prefer these to be in the form of: """Tested FileAttachment sandboxes MimetypeHandler.get_thumbnail""" And similar. The reason being that these … |
chipx86
|
|
|
This should be "Testing" (not "Tested") |
david
|
|
|
And here. |
david
|
|
|
And here. |
david
|
Summary: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 2 (+110 -4) |
-
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 -
-
-
-
-
-
reviewboard/attachments/tests.py (Diff revision 2) This isn't testing extension sandboxing.
Missing a trailing period.
-
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).
-
-
reviewboard/attachments/tests.py (Diff revision 2) We shouldn't be accessing private functions. Instead, access the property.
-
reviewboard/attachments/tests.py (Diff revision 2) As above, we should be using the property, not the private function.
-
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.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+86 -4) |
-
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 -
-
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+82 -4) |
-
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 -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+77 -3) |
-
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 -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+76 -3) |
-
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
-
-
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)
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+77 -3) |
-
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
Description: |
|
||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+76 -3) |
-
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
-
-
reviewboard/attachments/tests.py (Diff revision 8) Instead of creating a filediff and using
create_from_filediff, you can just callself.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.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+74 -3) |
-
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
-
-
-
-
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.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+76 -3) |
-
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
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+77 -3) |
-
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
-
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.
-
-
-
Description: |
|
|---|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (+77 -3) |
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