-
Notifications
You must be signed in to change notification settings - Fork 2
Add docstrings to tests #27
Changes from all commits
8d69656
ba0b4eb
4a3b2e5
d5572f6
c1931b3
e41a07b
0ed6ed6
db26c88
4046cbf
48c2004
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| class RouterTest(TestCase): | ||
| """Test that `node_router` correctly deals with the urls handed to it.""" | ||
| def test_root(self): | ||
| """node_router delegates to the Root Node's handler for an empty url.""" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests seem complicated - they were difficult to condense into docstring summaries. Perhaps they can be simplified?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They do. Not very DRY either. Perhaps if they were made dry, then it would look easier to have one assert per test here.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| url = '' | ||
| factories.NodeFactory.create() | ||
| request = mock.MagicMock() | ||
|
|
@@ -21,6 +22,7 @@ def test_root(self): | |
| self.assertEqual(response, handle(request, '/' + url)) | ||
|
|
||
| def test_complex_url(self): | ||
| """node_router finds the url's best match and delegates to its handler.""" | ||
| url = 'slug/42/foo/bar/' | ||
| factories.NodeFactory.create() | ||
| request = mock.MagicMock() | ||
|
|
@@ -35,6 +37,7 @@ def test_complex_url(self): | |
| class RouterIntegrationTest(TestCase): | ||
| """Test that `node_router` is correctly handed urls.""" | ||
| def test_root_url(self): | ||
| """The Root Node handler is passed the correct request and root url.""" | ||
| url = '/' | ||
| factories.NodeFactory.create() | ||
| handle_path = 'conman.nav_tree.models.Node.handle' | ||
|
|
@@ -45,6 +48,7 @@ def test_root_url(self): | |
| handle.assert_called_with(response.wsgi_request, url) | ||
|
|
||
| def test_complex_url(self): | ||
| """The correct request and url is passed through to the node's handler.""" | ||
| url = '/slug/42/foo/bar/' | ||
| factories.NodeFactory.create() | ||
| handle_path = 'conman.nav_tree.models.Node.handle' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,48 +12,36 @@ def setUp(self): | |
| self.request = self.create_request() | ||
| self.view = views.NodeRedirectView.as_view() | ||
|
|
||
| def test_target(self): | ||
| """NodeRedirectView redirects to the target's url.""" | ||
| node = ChildNodeRedirectFactory.create(target=self.target) | ||
| response = self.view(self.request, node=node) | ||
|
|
||
| self.assertEqual(response['Location'], self.target.url) | ||
|
|
||
| def test_permanent(self): | ||
| node = ChildNodeRedirectFactory.create( | ||
| target=self.target, | ||
| permanent=True, | ||
| ) | ||
| """A permanent redirect has status_code 301.""" | ||
| node = ChildNodeRedirectFactory.create(permanent=True) | ||
| response = self.view(self.request, node=node) | ||
|
|
||
| self.assertEqual(response.status_code, 301) | ||
| self.assertEqual(response['Location'], self.target.url) | ||
|
|
||
| def test_temporary(self): | ||
| node = ChildNodeRedirectFactory.create( | ||
| target=self.target, | ||
| permanent=False, | ||
| ) | ||
| """A temporary redirect has status_code 302.""" | ||
| node = ChildNodeRedirectFactory.create(permanent=False) | ||
| response = self.view(self.request, node=node) | ||
|
|
||
| self.assertEqual(response.status_code, 302) | ||
| self.assertEqual(response['Location'], self.target.url) | ||
|
|
||
|
|
||
| class TestNodeRedirectViewIntegration(TestCase): | ||
| def setUp(self): | ||
| self.target = ChildNodeFactory.create() | ||
| self.expected = 'http://testserver' + self.target.url | ||
|
|
||
| def test_permanent(self): | ||
| node = ChildNodeRedirectFactory.create( | ||
| target=self.target, | ||
| permanent=True, | ||
| ) | ||
| response = self.client.get(node.url) | ||
|
|
||
| self.assertEqual(response.status_code, 301) | ||
| self.assertEqual(response['Location'], self.expected) | ||
|
|
||
| def test_temporary(self): | ||
| node = ChildNodeRedirectFactory.create( | ||
| target=self.target, | ||
| permanent=False, | ||
| ) | ||
| def test_access_redirect(self): | ||
| """Accessing a NodeRedirect's url redirects to its target's url.""" | ||
| node = ChildNodeRedirectFactory.create(target=self.target) | ||
| response = self.client.get(node.url) | ||
|
|
||
| self.assertEqual(response.status_code, 302) | ||
| self.assertEqual(response['Location'], self.expected) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this refactor is an improvement, but if you disagree or think it shouldn't be in this pull-request I can easily remove it.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So do I, thanks |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected
ImportError. Is there a particular reasonValueErroris more appropriate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a peculiarity of implementation, I suppose. Perhaps we should not have this limitation (though I cannot see when we would need a top-level package here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Thinking about it a bit more, maybe
ValueErroris appropriate - the function is designed to work only with dotted paths. I think it should have a custom error message though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#28