djblets.datagrid.grids.Column sandboxing

Review Request #6502 — Created Oct. 24, 2014 and submitted

justy777
Djblets
master
f59f947...
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.

chipx86chipx86

These errors should ideally be in the form of: "Error when calling render_data for Datagrid Column %r: %s', self, e, ...

chipx86chipx86

Too generic for this. How about SandboxColumn?

chipx86chipx86

'DataGridColumnsHook' imported but unused

reviewbotreviewbot

This should go in the except block.

chipx86chipx86

"DataGrid", to match the class. Same below.

chipx86chipx86

The sort_field should go in the except block.

chipx86chipx86

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 ...

chipx86chipx86

Indentation issues. This code cannot execute. You're also missing all the setup needed to actually test sandboxing of this (see ...

chipx86chipx86

Col: 9 E112 expected an indented block

reviewbotreviewbot

Col: 9 E112 expected an indented block

reviewbotreviewbot

Sorry, probably a miscommunication on my part. What I'm looking for is: "'Error when calling render_data for DataGrid Column %r: ...

chipx86chipx86

get_toggle_url() isn't meant to be overridden by subclasses, so if there's a bug in it, we want that propagated.

chipx86chipx86

Same with get_header().

chipx86chipx86

Same with collect_objects().

chipx86chipx86

These aren't meant to be class variables. They must be passed to the constructor instead if the column is going ...

chipx86chipx86

These functions aren't meant to be overridden.

chipx86chipx86

This should have a period. (Note that this doesn't apply to the test docstrings below.)

chipx86chipx86

Can you use this format for the test docstrings: """Testing DataGrid column sandboxing for <func>"""

chipx86chipx86

Should be one line, with no parens.

chipx86chipx86

This should also say "... sandboxing column registration". Same below with unregistration.

chipx86chipx86

No blank line here.

chipx86chipx86

Here and below, we're not really sandboxing anything. We're just simply testing that the registration worked. Maybe: """Testing DataGridColumnsHook registeres ...

chipx86chipx86
reviewbot
  1. 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
    
    
  2. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
justy777
chipx86
  1. 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 DataGridColumnsHook is a thin wrapper around calling add_column or remove_column on 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 (through DataGridSubclass.add_column() -- note, not an instance).
    3) Whether DataGridSubclass.add_column() and DataGridSubclass.remove_column() are called when enabling/disabling an extension using a DataGridColumnsHook.

    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 that add_column() is called, and one to test that remove_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.

  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    Blank line between statements and blocks.

  3. 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.

  4. djblets/extensions/tests.py (Diff revision 2)
     
     

    Too generic for this. How about SandboxColumn?

  5. 
      
justy777
reviewbot
  1. 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
    
    
  2. djblets/extensions/tests.py (Diff revision 3)
     
     
     'DataGridColumnsHook' imported but unused
    
  3. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/extensions/tests.py
        djblets/datagrid/tests.py
    
    Ignored Files:
        AUTHORS
    
    
  2. djblets/extensions/tests.py (Diff revision 4)
     
     
    Col: 9
     E112 expected an indented block
    
  3. djblets/extensions/tests.py (Diff revision 4)
     
     
    Col: 9
     E112 expected an indented block
    
  4. 
      
justy777
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 4)
     
     

    This should go in the except block.

  3. djblets/datagrid/grids.py (Diff revision 4)
     
     

    "DataGrid", to match the class.

    Same below.

  4. djblets/datagrid/grids.py (Diff revision 4)
     
     
     

    The sort_field should go in the except block.

  5. 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.

  6. 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.

  7. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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
  2. 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'
    
  3. 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.

  4. djblets/datagrid/grids.py (Diff revision 7)
     
     
     
     
     
     

    Same with get_header().

  5. djblets/datagrid/grids.py (Diff revision 7)
     
     
     
     
     
     
     

    Same with collect_objects().

  6. 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?

  7. djblets/datagrid/tests.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    These functions aren't meant to be overridden.

  8. djblets/datagrid/tests.py (Diff revision 7)
     
     

    This should have a period.

    (Note that this doesn't apply to the test docstrings below.)

  9. djblets/datagrid/tests.py (Diff revision 7)
     
     

    Can you use this format for the test docstrings:

    """Testing DataGrid column sandboxing for <func>"""
    
  10. djblets/extensions/tests.py (Diff revision 7)
     
     
     

    Should be one line, with no parens.

  11. 
      
chipx86
  1. 
      
  2. djblets/extensions/tests.py (Diff revision 7)
     
     

    This should also say "... sandboxing column registration".

    Same below with unregistration.

  3. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
justy777
chipx86
  1. Great description and testing text!

    This is basically done. A couple small things, and we'll be ready to ship it :)

  2. djblets/datagrid/tests.py (Diff revision 8)
     
     
     
     

    No blank line here.

  3. 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"""
    
  4. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Ship It!

  2. 
      
justy777
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.8.x (f9440ba)
Loading...