launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07404
[Merge] lp:~jtv/maas/bug-989384 into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-989384 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #989384 in MAAS: "Test helper: extract path from redirect response"
https://bugs.launchpad.net/maas/+bug/989384
For more details, see:
https://code.launchpad.net/~jtv/maas/bug-989384/+merge/103866
This will save us some typing (and cargo-culting, not to mention reading time) from tests that need to check out redirects (HttpResponses with status code httplib.FOUND) and see where they redirect to.
Note how the way I put that asserts that the redirect is really a response. If that's what it's supposed to be, just use this function with confidence. But if it's not a redirect, you'll get a reasonably helpful message about what was in the response. No more need to check its status code in your test, let alone fire up a debugger to get at the response body!
--
https://code.launchpad.net/~jtv/maas/bug-989384/+merge/103866
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-989384 into lp:maas.
=== modified file 'src/maasserver/testing/__init__.py'
--- src/maasserver/testing/__init__.py 2012-04-23 11:34:19 +0000
+++ src/maasserver/testing/__init__.py 2012-04-27 12:45:26 +0000
@@ -11,6 +11,7 @@
__metaclass__ = type
__all__ = [
+ "extract_redirect",
"get_content_links",
"get_data",
"get_fake_provisioning_api_proxy",
@@ -19,7 +20,9 @@
"reload_objects",
]
+import httplib
import os
+from urlparse import urlparse
from uuid import uuid1
from lxml.html import fromstring
@@ -59,6 +62,29 @@
fake_provisioning_proxy = None
+def extract_redirect(http_response):
+ """Extract redirect target from an http response object.
+
+ Only the http path part of the redirect is ignored; protocol and host
+ name, if present, are not included in the result.
+
+ If the response is not a redirect, this raises :class:`ValueError` with
+ a descriptive error message.
+
+ :param http_response: A response returned from an http request.
+ :type http_response: :class:`HttpResponse`
+ :return: The "path" part of the target that `http_response` redirects to.
+ :raises: ValueError
+ """
+ if http_response.status_code != httplib.FOUND:
+ raise ValueError(
+ "Not a redirect: http status %d. Content: %s"
+ % (http_response.status_code, http_response.content[:80]))
+ target_url = http_response['Location']
+ parsed_url = urlparse(target_url)
+ return parsed_url.path
+
+
def reload_object(model_object):
"""Reload `obj` from the database.
=== modified file 'src/maasserver/testing/tests/test_module.py'
--- src/maasserver/testing/tests/test_module.py 2012-04-16 10:00:51 +0000
+++ src/maasserver/testing/tests/test_module.py 2012-04-27 12:45:26 +0000
@@ -12,11 +12,19 @@
__metaclass__ = type
__all__ = []
+import httplib
+
+from django.http import (
+ HttpResponse,
+ HttpResponseRedirect,
+ )
from maasserver import provisioning
from maasserver.testing import (
+ extract_redirect,
reload_object,
reload_objects,
)
+from maasserver.testing.factory import factory
from maasserver.testing.models import TestModel
from maasserver.testing.testcase import (
TestCase,
@@ -76,6 +84,29 @@
app = 'maasserver.testing'
+ def test_extract_redirect_extracts_redirect_location(self):
+ url = factory.getRandomString()
+ self.assertEqual(
+ url, extract_redirect(HttpResponseRedirect(url)))
+
+ def test_extract_redirect_only_returns_target_path(self):
+ url_path = factory.getRandomString()
+ self.assertEqual(
+ "/%s" % url_path,
+ extract_redirect(
+ HttpResponseRedirect("http://example.com/%s"; % url_path)))
+
+ def test_extract_redirect_errors_out_helpfully_if_not_a_redirect(self):
+ content = factory.getRandomString(10)
+ other_response = HttpResponse(status=httplib.OK, content=content)
+ try:
+ extract_redirect(other_response)
+ except ValueError as e:
+ pass
+
+ self.assertIn(unicode(httplib.OK), unicode(e))
+ self.assertIn(content, unicode(e))
+
def test_reload_object_reloads_object(self):
test_obj = TestModel(text="old text")
test_obj.save()
=== modified file 'src/maasserver/tests/test_exceptions.py'
--- src/maasserver/tests/test_exceptions.py 2012-04-25 16:19:07 +0000
+++ src/maasserver/tests/test_exceptions.py 2012-04-27 12:45:26 +0000
@@ -18,6 +18,7 @@
MAASAPIBadRequest,
Redirect,
)
+from maasserver.testing import extract_redirect
from maastesting.factory import factory
from maastesting.testcase import TestCase
@@ -36,5 +37,4 @@
target = factory.getRandomString()
exception = Redirect(target)
response = exception.make_http_response()
- self.assertEqual(httplib.FOUND, response.status_code)
- self.assertEqual(target, response['Location'])
+ self.assertEqual(target, extract_redirect(response))
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-04-27 10:56:37 +0000
+++ src/maasserver/tests/test_forms.py 2012-04-27 12:45:26 +0000
@@ -42,10 +42,15 @@
DEFAULT_CONFIG,
MACAddress,
)
+<<<<<<< TREE
from maasserver.node_action import (
AcceptAndCommission,
Delete,
)
+=======
+from maasserver.provisioning import get_provisioning_api_proxy
+from maasserver.testing import extract_redirect
+>>>>>>> MERGE-SOURCE
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
from provisioningserver.enum import POWER_TYPE_CHOICES
@@ -243,6 +248,104 @@
class TestNodeActionForm(TestCase):
+<<<<<<< TREE
+=======
+ def test_inhibit_delete_allows_deletion_of_unowned_node(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ self.assertIsNone(inhibit_delete(node, admin))
+
+ def test_inhibit_delete_inhibits_deletion_of_owned_node(self):
+ admin = factory.make_admin()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ self.assertEqual(
+ "You cannot delete this node because it's in use.",
+ inhibit_delete(node, admin))
+
+ def test_delete_node_redirects_to_confirmation_page(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ try:
+ delete_node(node, admin)
+ except MAASAPIException as e:
+ pass
+ else:
+ self.fail("delete_node should have raised a Redirect.")
+ response = e.make_http_response()
+ self.assertEqual(
+ reverse('node-delete', args=[node.system_id]),
+ extract_redirect(response))
+
+ def have_permission_returns_False_if_action_is_None(self):
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ self.assertFalse(get_action_form(admin)(node).have_permission(None))
+
+ def have_permission_returns_False_if_lacking_permission(self):
+ user = factory.make_user()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ action = {
+ 'permission': NODE_PERMISSION.EDIT,
+ }
+ self.assertFalse(get_action_form(user)(node).have_permission(action))
+
+ def have_permission_returns_True_given_permission(self):
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ action = {
+ 'permission': NODE_PERMISSION.EDIT,
+ }
+ form = get_action_form(node.owner)(node)
+ self.assertTrue(form.have_permission(action))
+
+ def test_compile_action_records_inhibition(self):
+ node = factory.make_node()
+ user = factory.make_user()
+ form = get_action_form(user)(node)
+ inhibition = factory.getRandomString()
+
+ def inhibit(*args, **kwargs):
+ return inhibition
+
+ compiled_action = form.compile_action({'inhibit': inhibit})
+ self.assertEqual(inhibition, compiled_action['inhibition'])
+
+ def test_compile_action_records_None_if_no_inhibition(self):
+ node = factory.make_node()
+ user = factory.make_user()
+ form = get_action_form(user)(node)
+
+ def inhibit(*args, **kwargs):
+ return None
+
+ compiled_action = form.compile_action({'inhibit': inhibit})
+ self.assertIsNone(compiled_action['inhibition'])
+
+ def test_compile_actions_for_declared_node_admin(self):
+ # Check which transitions are available for an admin on a
+ # 'Declared' node.
+ admin = factory.make_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_action_form(admin)(node)
+ actions = form.compile_actions()
+ self.assertItemsEqual(
+ ["Accept & commission", "Delete node"],
+ [action['display'] for action in actions])
+ # All permissions should be ADMIN.
+ self.assertEqual(
+ [NODE_PERMISSION.ADMIN] * len(actions),
+ [action['permission'] for actions in actions])
+
+ def test_compile_actions_for_declared_node_simple_user(self):
+ # A simple user sees no actions for a 'Declared' node.
+ user = factory.make_user()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ form = get_action_form(user)(node)
+ self.assertItemsEqual([], form.compile_actions())
+
+>>>>>>> MERGE-SOURCE
def test_get_action_form_creates_form_class_with_attributes(self):
user = factory.make_admin()
form_class = get_action_form(user)
=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py 2012-04-20 12:54:26 +0000
+++ src/maasserver/tests/test_middleware.py 2012-04-27 12:45:26 +0000
@@ -44,6 +44,7 @@
ExternalComponentsMiddleware,
PROFILES_CHECK_DONE_KEY,
)
+from maasserver.testing import extract_redirect
from maasserver.testing.factory import factory
from maasserver.testing.testcase import (
LoggedInTestCase,
@@ -301,9 +302,7 @@
middleware = ErrorsMiddleware()
response = middleware.process_exception(request, exception)
# The response is a redirect.
- self.assertEqual(
- (httplib.FOUND, response['Location']),
- (response.status_code, url))
+ self.assertEqual(url, extract_redirect(response))
# An error message has been published.
self.assertEqual(
[(constants.ERROR, error_message, '')], request._messages.messages)
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-23 14:02:51 +0000
+++ src/maasserver/tests/test_views.py 2012-04-27 12:45:26 +0000
@@ -13,7 +13,6 @@
__all__ = []
import httplib
-from urlparse import urlparse
from xmlrpclib import Fault
from django.conf.urls.defaults import patterns
@@ -26,6 +25,7 @@
from maasserver import components
from maasserver.components import register_persistent_error
from maasserver.exceptions import ExternalComponentException
+from maasserver.testing import extract_redirect
from maasserver.testing.enum import map_enum
from maasserver.testing.factory import factory
from maasserver.testing.testcase import (
@@ -189,9 +189,7 @@
request = RequestFactory().get('/foo')
view = FakeDeleteView(next_url=next_url, request=request)
response = view.get(request)
- self.assertEqual(
- (httplib.FOUND, next_url),
- (response.status_code, response['Location']))
+ self.assertEqual(next_url, extract_redirect(response))
self.assertEqual([view.compose_feedback_nonexistent()], view.notices)
def test_compose_feedback_nonexistent_names_class(self):
@@ -224,10 +222,7 @@
node = factory.make_node(owner=self.logged_in_user)
node_edit_link = reverse('node-edit', args=[node.system_id])
response = self.client.post(node_edit_link, {})
- redirect_url = urlparse(response['Location']).path
- self.assertEqual(
- (httplib.FOUND, redirect_url),
- (response.status_code, node_edit_link))
+ self.assertEqual(node_edit_link, extract_redirect(response))
def test_raised_ExternalComponentException_publishes_message(self):
# When a ExternalComponentException is raised in a POST request, a
=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py 2012-04-27 10:56:37 +0000
+++ src/maasserver/tests/test_views_nodes.py 2012-04-27 12:45:26 +0000
@@ -13,7 +13,6 @@
__all__ = []
import httplib
-from urlparse import urlparse
from django.conf import settings
from django.core.urlresolvers import reverse
@@ -32,6 +31,7 @@
Node,
)
from maasserver.testing import (
+ extract_redirect,
get_content_links,
reload_object,
reload_objects,
@@ -300,20 +300,70 @@
factory.make_sshkey(self.logged_in_user)
self.set_up_oauth_token()
node = factory.make_node(status=NODE_STATUS.READY)
+<<<<<<< TREE
node_link = reverse('node-view', args=[node.system_id])
response = self.client.post(
node_link, data={NodeActionForm.input_name: StartNode.display})
self.assertEqual(httplib.FOUND, response.status_code)
self.assertEqual(NODE_STATUS.ALLOCATED, reload_object(node).status)
+=======
+ response = self.client.post(
+ reverse('node-view', args=[node.system_id]),
+ data={NodeActionForm.input_name: "Delete node"})
+ self.assertEqual(
+ reverse('node-delete', args=[node.system_id]),
+ extract_redirect(response))
+
+ def test_view_node_POST_admin_cannot_delete_used_node(self):
+ self.become_admin()
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ response = self.client.post(
+ reverse('node-view', args=[node.system_id]),
+ data={NodeActionForm.input_name: "Delete node"})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+ def test_view_node_POST_admin_can_start_commissioning_node(self):
+ self.become_admin()
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ node_link = reverse('node-view', args=[node.system_id])
+ response = self.client.post(
+ node_link,
+ data={
+ NodeActionForm.input_name: "Accept & commission",
+ })
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertEqual(
+ NODE_STATUS.COMMISSIONING, reload_object(node).status)
+
+ def test_view_node_POST_admin_can_retry_failed_commissioning(self):
+ self.become_admin()
+ node = factory.make_node(status=NODE_STATUS.FAILED_TESTS)
+ node_link = reverse('node-view', args=[node.system_id])
+ response = self.client.post(
+ node_link,
+ data={NodeActionForm.input_name: "Retry commissioning"})
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertEqual(
+ NODE_STATUS.COMMISSIONING, reload_object(node).status)
+>>>>>>> MERGE-SOURCE
def perform_action_and_get_node_page(self, node, action_name):
"""POST to perform a node action, then load the resulting page."""
node_link = reverse('node-view', args=[node.system_id])
response = self.client.post(
+<<<<<<< TREE
node_link, data={NodeActionForm.input_name: action_name})
if response.status_code != httplib.FOUND:
self.fail("%d: '%s'" % (response.status_code, response.content))
redirect = urlparse(response['Location']).path
+=======
+ node_link,
+ data={
+ NodeActionForm.input_name: action_name,
+ })
+ redirect = extract_redirect(response)
+>>>>>>> MERGE-SOURCE
if redirect != node_link:
self.fail("Odd: %s redirected to %s." % (node_link, redirect))
return self.client.get(redirect)
@@ -355,10 +405,9 @@
mac = factory.getRandomMACAddress()
mac_delete_link = reverse('mac-delete', args=[node.system_id, mac])
response = self.client.get(mac_delete_link)
- next_url = reverse('node-edit', args=[node.system_id])
self.assertEqual(
- (httplib.FOUND, next_url),
- (response.status_code, urlparse(response['Location']).path))
+ reverse('node-edit', args=[node.system_id]),
+ extract_redirect(response))
def test_node_delete_access_denied_if_user_cannot_edit_node(self):
node = factory.make_node(owner=factory.make_user())
@@ -382,10 +431,9 @@
mac = factory.make_mac_address(node=node)
mac_delete_link = reverse('mac-delete', args=[node.system_id, mac])
response = self.client.post(mac_delete_link, {'post': 'yes'})
- next_url = reverse('node-edit', args=[node.system_id])
self.assertEqual(
- (httplib.FOUND, next_url),
- (response.status_code, urlparse(response['Location']).path))
+ reverse('node-edit', args=[node.system_id]),
+ extract_redirect(response))
self.assertFalse(MACAddress.objects.filter(id=mac.id).exists())
def test_node_delete_mac_POST_displays_message(self):
@@ -393,7 +441,8 @@
mac = factory.make_mac_address(node=node)
mac_delete_link = reverse('mac-delete', args=[node.system_id, mac])
response = self.client.post(mac_delete_link, {'post': 'yes'})
- response = self.client.get(urlparse(response['Location']).path)
+ redirect = extract_redirect(response)
+ response = self.client.get(redirect)
self.assertEqual(
["Mac address %s deleted." % mac.mac_address],
[message.message for message in response.context['messages']])
@@ -413,10 +462,9 @@
mac_add_link = reverse('mac-add', args=[node.system_id])
mac = factory.getRandomMACAddress()
response = self.client.post(mac_add_link, {'mac_address': mac})
- next_url = reverse('node-edit', args=[node.system_id])
self.assertEqual(
- (httplib.FOUND, next_url),
- (response.status_code, urlparse(response['Location']).path))
+ reverse('node-edit', args=[node.system_id]),
+ extract_redirect(response))
self.assertTrue(
MACAddress.objects.filter(node=node, mac_address=mac).exists())
@@ -425,7 +473,8 @@
mac_add_link = reverse('mac-add', args=[node.system_id])
mac = factory.getRandomMACAddress()
response = self.client.post(mac_add_link, {'mac_address': mac})
- response = self.client.get(urlparse(response['Location']).path)
+ redirect = extract_redirect(response)
+ response = self.client.get(redirect)
self.assertEqual(
["MAC address added."],
[message.message for message in response.context['messages']])
=== modified file 'src/maasserver/tests/test_views_prefs.py'
--- src/maasserver/tests/test_views_prefs.py 2012-04-23 11:34:19 +0000
+++ src/maasserver/tests/test_views_prefs.py 2012-04-27 12:45:26 +0000
@@ -14,13 +14,13 @@
import httplib
-from urlparse import urlparse
from django.contrib.auth.models import User
from django.core.urlresolvers import reverse
from lxml.html import fromstring
from maasserver.models import SSHKey
from maasserver.testing import (
+ extract_redirect,
get_content_links,
get_data,
get_prefixed_form_data,
@@ -196,9 +196,7 @@
del_link = reverse('prefs-delete-sshkey', args=[key.id])
key.delete()
response = self.client.get(del_link)
- self.assertEqual(
- (httplib.FOUND, '/account/prefs/'),
- (response.status_code, urlparse(response['Location']).path))
+ self.assertEqual('/account/prefs/', extract_redirect(response))
def test_delete_key_POST(self):
# A POST request deletes the key, and redirects to the prefs.
@@ -206,9 +204,7 @@
del_link = reverse('prefs-delete-sshkey', args=[key.id])
response = self.client.post(del_link, {'post': 'yes'})
- self.assertEqual(
- (httplib.FOUND, '/account/prefs/'),
- (response.status_code, urlparse(response['Location']).path))
+ self.assertEqual('/account/prefs/', extract_redirect(response))
self.assertFalse(SSHKey.objects.filter(id=key.id).exists())
def test_delete_key_POST_ignores_nonexistent_key(self):
@@ -218,6 +214,4 @@
del_link = reverse('prefs-delete-sshkey', args=[key.id])
key.delete()
response = self.client.post(del_link, {'post': 'yes'})
- self.assertEqual(
- (httplib.FOUND, '/account/prefs/'),
- (response.status_code, urlparse(response['Location']).path))
+ self.assertEqual('/account/prefs/', extract_redirect(response))
