djblets.datagrid.grids.Column sandboxing
Review Request #6502 — Created Oct. 24, 2014 and submitted
| Information | |
|---|---|
| justy777 | |
| Djblets | |
| master | |
| f59f947... | |
| Reviewers | |
| djblets, students | |
Extensions that create a Column subclass (using the DataGridColumnsHook) can throw exceptions inside Djblets. The DataGridColumnsHook hook and the parts of Djblets that call those methods have been sandboxed.
Now when a Column subclass from an extension throws an exception; Djblets logs the errors with enough information to find which method in the Column subclass threw the exception.
Unit tests have been written to make sure that functions from a Column subclass have been called, and when an exception is thrown it gets logged. Tests for DataGridColumnsHook test that the Column subclass when an exception is thrown while registering and unregistering a Column subclass it gets logged.
The tests fail without the sanboxing, and succeed with it.
| Description | From | Last Updated |
|---|---|---|
|
Blank line between statements and blocks. |
 chipx86
|
|
|
These errors should ideally be in the form of: "Error when calling render_data for Datagrid Column %r: %s', self, e, … |
chipx86
|
|
|
Too generic for this. How about SandboxColumn? |
chipx86
|
|
|
'DataGridColumnsHook' imported but unused |
 reviewbot
|
|
|
This should go in the except block. |
chipx86
|
|
|
"DataGrid", to match the class. Same below. |
chipx86
|
|
|
The sort_field should go in the except block. |
chipx86
|
|
|
Extensions aren't needed for the datagrid-only tests. Remember that the hook just wraps the DataGrid.add_column and remove_column functions, which you … |
chipx86
|
|
|
Indentation issues. This code cannot execute. You're also missing all the setup needed to actually test sandboxing of this (see … |
chipx86
|
|
|
Col: 9 E112 expected an indented block |
reviewbot
|
|
|
Col: 9 E112 expected an indented block |
reviewbot
|
|
|
Sorry, probably a miscommunication on my part. What I'm looking for is: "'Error when calling render_data for DataGrid Column %r: … |
chipx86
|
|
|
get_toggle_url() isn't meant to be overridden by subclasses, so if there's a bug in it, we want that propagated. |
chipx86
|
|
|
Same with get_header(). |
chipx86
|
|
|
Same with collect_objects(). |
chipx86
|
|
|
These aren't meant to be class variables. They must be passed to the constructor instead if the column is going … |
chipx86
|
|
|
These functions aren't meant to be overridden. |
chipx86
|
|
|
This should have a period. (Note that this doesn't apply to the test docstrings below.) |
chipx86
|
|
|
Can you use this format for the test docstrings: """Testing DataGrid column sandboxing for <func>""" |
chipx86
|
|
|
Should be one line, with no parens. |
chipx86
|
|
|
This should also say "... sandboxing column registration". Same below with unregistration. |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
Here and below, we're not really sandboxing anything. We're just simply testing that the registration worked. Maybe: """Testing DataGridColumnsHook registeres … |
chipx86
|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+224 -7) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py Ignored Files: AUTHORS
Description: |
|
|---|
-
Some general comments for the unit tests.
All your tests are going through the work of setting up an extension and registering a column class. Then it completely disregards this and operates directly on the column class and datagrid. There's no point to the extension setup here.
The
DataGridColumnsHookis a thin wrapper around callingadd_columnorremove_columnon the datagrid. Then, everything is done through functions called on the datagrid instance.So really, two three of things to test:
1) Functions on a Column that are called when calling functions on a DataGrid instance.
2) Functions on a Column that are called when adding a colunm to a DataGrid subclass (throughDataGridSubclass.add_column()-- note, not an instance).
3) WhetherDataGridSubclass.add_column()andDataGridSubclass.remove_column()are called when enabling/disabling an extension using aDataGridColumnsHook.The first two tests belong in
datagrid/tests.py, as they are independent of extensions and hooks.The third test belongs in
extensions/tests.py, but should be very simple. Two test functions. One to test thatadd_column()is called, and one to test thatremove_column()is called. You can easily test these by establishing a function spy using kgb (see docs at https://github.com/beanbaginc/kgb), enabling/disabling the extension (depending on the test), and checking if the function was called with the expected class. No need to test beyond that for hooks, since that's all a hook is responsible for in this case. -
-
djblets/datagrid/grids.py (Diff revision 2) These errors should ideally be in the form of:
"Error when calling render_data for Datagrid Column %r: %s', self, e, exc_info=1)By using %r, we have more information about what this column is. Also notice "Datagrid Column" vs "column".
Same below.
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+242 -8) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+222 -8) |
-
Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS -
-
Description: |
|
|---|
-
-
-
-
-
djblets/datagrid/tests.py (Diff revision 4) Extensions aren't needed for the datagrid-only tests. Remember that the hook just wraps the DataGrid.add_column and remove_column functions, which you should use directly in the tests.
Unit tests should be as simple as they can be to properly test what they need to test. The only unit tests in this change that needs a SandboxExtension are the ones that are testing that whether the hook properly calls the add/remove_column functions.
-
djblets/extensions/tests.py (Diff revision 4) Indentation issues. This code cannot execute.
You're also missing all the setup needed to actually test sandboxing of this (see my note on kgb), and the proper parameters to DataGridColumnsHook.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+211 -8) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+224 -17) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+228 -17) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS
-
I realized we're sandboxing some functions that aren't overridden by subclasses, which we don't need to sandbox or test for. I've marked them below.
One more note that applies generally to changes. We'd like the description of the review request, which is used as the basis for the commit message, to have a bit more explanation for the purpose of the change and what was done. For some examples, see:
- https://github.com/reviewboard/reviewboard/commit/52f70904afc654668b7f19825f1f938281e8634d
- https://github.com/reviewboard/reviewboard/commit/b6057662802217587d7935bc5646234b6e69308f
-
djblets/datagrid/grids.py (Diff revision 7) Sorry, probably a miscommunication on my part. What I'm looking for is:
"'Error when calling render_data for DataGrid Column %r: %s' -
djblets/datagrid/grids.py (Diff revision 7) get_toggle_url()isn't meant to be overridden by subclasses, so if there's a bug in it, we want that propagated. -
-
-
djblets/datagrid/tests.py (Diff revision 7) These aren't meant to be class variables. They must be passed to the constructor instead if the column is going to use them (since they'll be overridden otherwise).
However, for the tests, I don't think they're even needed for anything?
-
-
djblets/datagrid/tests.py (Diff revision 7) This should have a period.
(Note that this doesn't apply to the test docstrings below.)
-
djblets/datagrid/tests.py (Diff revision 7) Can you use this format for the test docstrings:
"""Testing DataGrid column sandboxing for <func>""" -
-
-
djblets/extensions/tests.py (Diff revision 7) This should also say "... sandboxing column registration".
Same below with unregistration.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+156 -14) |
-
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py djblets/datagrid/tests.py Ignored Files: AUTHORS
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
Great description and testing text!
This is basically done. A couple small things, and we'll be ready to ship it :)
-
-
djblets/extensions/tests.py (Diff revision 8) Here and below, we're not really sandboxing anything. We're just simply testing that the registration worked. Maybe:
"""Testing DataGridColumnsHook registeres column"""
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+155 -14) |
Tool: Pyflakes Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py Ignored Files: AUTHORS Tool: PEP8 Style Checker Processed Files: djblets/datagrid/grids.py djblets/extensions/tests.py Ignored Files: AUTHORS