Summary

djblets.datagrid.grids.Column sandboxing

Review Request #6502 — Created Oct. 24, 2014 and submitted Nov. 1, 2014, 4:25 p.m.

Information

Owner

justy777

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

f59f947...

Reviewers

Groups

djblets, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Blank line between statements and blocks.	chipx86	Oct. 27, 2014, 12:33 p.m.
These errors should ideally be in the form of: "Error when calling render_data for Datagrid Column %r: %s', self, e, …	chipx86	Oct. 27, 2014, 12:40 p.m.
Too generic for this. How about SandboxColumn?	chipx86	Oct. 27, 2014, 12:41 p.m.
'DataGridColumnsHook' imported but unused	reviewbot	Oct. 27, 2014, 2:10 p.m.
This should go in the except block.	chipx86	Oct. 28, 2014, 9:39 a.m.
"DataGrid", to match the class. Same below.	chipx86	Oct. 28, 2014, 9:49 a.m.
The sort_field should go in the except block.	chipx86	Oct. 28, 2014, 9:42 a.m.
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	Oct. 28, 2014, 9:40 a.m.
Indentation issues. This code cannot execute. You're also missing all the setup needed to actually test sandboxing of this (see …	chipx86	Oct. 28, 2014, 10:37 a.m.
Col: 9 E112 expected an indented block	reviewbot	Oct. 28, 2014, 9:42 a.m.
Col: 9 E112 expected an indented block	reviewbot	Oct. 28, 2014, 9:42 a.m.
Sorry, probably a miscommunication on my part. What I'm looking for is: "'Error when calling render_data for DataGrid Column %r: …	chipx86	Oct. 30, 2014, 12:18 a.m.
get_toggle_url() isn't meant to be overridden by subclasses, so if there's a bug in it, we want that propagated.	chipx86	Oct. 30, 2014, 12:22 a.m.
Same with get_header().	chipx86	Oct. 30, 2014, 12:22 a.m.
Same with collect_objects().	chipx86	Oct. 30, 2014, 12:22 a.m.
These aren't meant to be class variables. They must be passed to the constructor instead if the column is going …	chipx86	Oct. 30, 2014, 12:22 a.m.
These functions aren't meant to be overridden.	chipx86	Oct. 30, 2014, 12:23 a.m.
This should have a period. (Note that this doesn't apply to the test docstrings below.)	chipx86	Oct. 30, 2014, 12:23 a.m.
Can you use this format for the test docstrings: """Testing DataGrid column sandboxing for <func>"""	chipx86	Oct. 30, 2014, 12:25 a.m.
Should be one line, with no parens.	chipx86	Oct. 30, 2014, 12:28 a.m.
This should also say "... sandboxing column registration". Same below with unregistration.	chipx86	Oct. 30, 2014, 12:28 a.m.
No blank line here.	chipx86	Nov. 1, 2014, 4:15 p.m.
Here and below, we're not really sandboxing anything. We're just simply testing that the registration worked. Maybe: """Testing DataGridColumnsHook registeres …	chipx86	Nov. 1, 2014, 4:17 p.m.

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

Commit:

-	9a5b5fef008444b1007eb2de3d054a729d7999a8
+	e7dbd29586624261946e183d777663e007ffe0c5

Diff:

Revision 2 (+224 -7)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/extensions/tests.py

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:

		Unit tests for djblets.datagrid.grids.Column, and then sandboxing call to methods on the djblets.datagrid.grids.Column subclass

		methods sandboxed:
~		setup_state()
	~	setup_state()
	+	get_toggle_url()
	+	get_header()
	+	render_data()
	+	augment_queryset()

		TODO:
		Write tests for:
		get_sort_field()
-		get_toggle_url()
-		get_header()
		collect_objects()
~		render_cell()
	~	render_cell()
-		render_data()
-		augment_queryset()

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.

djblets/datagrid/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between statements and blocks.
```

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

The issue has been resolved. 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.

djblets/extensions/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Too generic for this. How about SandboxColumn?
```

Commit:

-	e7dbd29586624261946e183d777663e007ffe0c5
+	80ccd3d2e78b09bffcd5be317652d1fb5ce729cd

Diff:

Revision 3 (+242 -8)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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

djblets/extensions/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 'DataGridColumnsHook' imported but unused
```

Commit:

-	80ccd3d2e78b09bffcd5be317652d1fb5ce729cd
+	445bf92b0aafb9f6cd08ec93aa6eb0e0c9c0d1e7

Diff:

Revision 4 (+222 -8)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/datagrid/grids.py
    djblets/extensions/tests.py
    djblets/datagrid/tests.py

Ignored Files:
    AUTHORS

djblets/extensions/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 9
 E112 expected an indented block
```
djblets/extensions/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 9
 E112 expected an indented block
```

Description:

		Unit tests for djblets.datagrid.grids.Column, and then sandboxing call to methods on the djblets.datagrid.grids.Column subclass

		methods sandboxed:
		setup_state()
		get_toggle_url()
		get_header()
		render_data()
		augment_queryset()

		TODO:
		Write tests for:
	+	add_column()
	+	remove_column()
		get_sort_field()
		collect_objects()
		render_cell()

djblets/datagrid/grids.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should go in the except block.
```
djblets/datagrid/grids.py (Diff revision 4)
The issue has been resolved. Show all issues
```
"DataGrid", to match the class.
Same below.
```
djblets/datagrid/grids.py (Diff revision 4)
The issue has been resolved. Show all issues
```
The sort_field should go in the except block.
```

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

The issue has been resolved. 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.

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

The issue has been resolved. 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.

Description:

		Unit tests for djblets.datagrid.grids.Column, and then sandboxing call to methods on the djblets.datagrid.grids.Column subclass

	+	Hook sandboxed: DataGridColumnsHook
	+
		methods sandboxed:
		setup_state()
		get_toggle_url()
		get_header()
		render_data()
		augment_queryset()

~		TODO:
	~	TODO: - Write tests for:
-		Write tests for:
-		add_column()
-		remove_column()
		get_sort_field()
		collect_objects()
		render_cell()

Commit:

-	445bf92b0aafb9f6cd08ec93aa6eb0e0c9c0d1e7
+	6d3b5e2e0fd5143a0b9477a95f7b361e0a820a7c

Diff:

Revision 5 (+211 -8)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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:

-	[WIP] djblets.datagrid.grids.Column sandboxing
+	djblets.datagrid.grids.Column sandboxing

Description:

		Unit tests for djblets.datagrid.grids.Column, and then sandboxing call to methods on the djblets.datagrid.grids.Column subclass

		Hook sandboxed: DataGridColumnsHook

		methods sandboxed:
		setup_state()
	+	get_sort_field()
		get_toggle_url()
		get_header()
	+	collect_objects()
	+	render_cell()
		render_data()
		augment_queryset()
-
-		TODO: - Write tests for:
-		get_sort_field()
-		collect_objects()
-		render_cell()

Testing Done:

~		Ran unit tests.
	~	Ran unit tests; fail initially and succeed after sandboxing.

Commit:

-	6d3b5e2e0fd5143a0b9477a95f7b361e0a820a7c
+	354fc97ead4d116e4b2e387d12a692a9139789b8

Diff:

Revision 6 (+224 -17)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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:

-	354fc97ead4d116e4b2e387d12a692a9139789b8
+	db9f72177d3ed830cf8cc5bb78530715076c1358

Diff:

Revision 7 (+228 -17)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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)

The issue has been resolved. 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'

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

The issue has been resolved. 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.

djblets/datagrid/grids.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Same with get_header().
```
djblets/datagrid/grids.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Same with collect_objects().
```

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

The issue has been resolved. 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?

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

The issue has been resolved. Show all issues

These functions aren't meant to be overridden.

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

The issue has been resolved. Show all issues

This should have a period.

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

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

The issue has been resolved. Show all issues

Can you use this format for the test docstrings:

"""Testing DataGrid column sandboxing for <func>"""

djblets/extensions/tests.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Should be one line, with no parens.
```

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

The issue has been resolved. Show all issues

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

Same below with unregistration.

Commit:

-	db9f72177d3ed830cf8cc5bb78530715076c1358
+	e586b2718ef083d3f8ff338bab394fdd1c29f90a

Diff:

Revision 8 (+156 -14)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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:

~		Unit tests for djblets.datagrid.grids.Column, and then sandboxing call to methods on the djblets.datagrid.grids.Column subclass
	~	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.

~		Hook sandboxed: DataGridColumnsHook
	~	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.
-
-		methods sandboxed:
-		setup_state()
-		get_sort_field()
-		get_toggle_url()
-		get_header()
-		collect_objects()
-		render_cell()
-		render_data()
-		augment_queryset()

Testing Done:

~		Ran unit tests; fail initially and succeed after sandboxing.
	~	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.

Great description and testing text!

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

djblets/datagrid/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line here.
```

djblets/extensions/tests.py (Diff revision 8)

The issue has been resolved. 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"""

Commit:

-	e586b2718ef083d3f8ff338bab394fdd1c29f90a
+	f59f9473a62362f14b72ea9befaacf4a8a7ccc6a

Diff:

Revision 9 (+155 -14)

Show changes

	AUTHORS
	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/extensions/tests.py

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.8.x (f9440ba)