djblets.datagrid.grids.Column sandboxing

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

Information

Djblets
master
f59f947...

Reviewers

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)
     
     
     
    Show all issues

    Blank line between statements and blocks.

  3. djblets/datagrid/grids.py (Diff revision 2)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues
     '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)
     
     
    Show all issues
    Col: 9
     E112 expected an indented block
    
  3. djblets/extensions/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 9
     E112 expected an indented block
    
  4. 
      
justy777
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 4)
     
     
    Show all issues

    This should go in the except block.

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

    "DataGrid", to match the class.

    Same below.

  4. djblets/datagrid/grids.py (Diff revision 4)
     
     
     
    Show all issues

    The sort_field should go in the except block.

  5. djblets/datagrid/tests.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    Same with get_header().

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

    Same with collect_objects().

  6. djblets/datagrid/tests.py (Diff revision 7)
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These functions aren't meant to be overridden.

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

    This should have a period.

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

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

    Can you use this format for the test docstrings:

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

    Should be one line, with no parens.

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

    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)
     
     
     
     
    Show all issues

    No blank line here.

  3. djblets/extensions/tests.py (Diff revision 8)
     
     
    Show all issues

    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:
Completed
Change Summary:
Pushed to release-0.8.x (f9440ba)