[Django] #29098: Allow assertRedirects to handle regex matches.
Django
-------------------------------------+-------------------------------------
Reporter: Dan J | Owner: nobody
Strohl |
Type: | Status: new
Cleanup/optimization |
Component: Testing | Version: 1.11
framework |
Severity: Normal | Keywords: unittest redirect
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
or, perhaps, allow it to use the patterns from the url's file. Either
way, the issue is that I have a view that gets a request, looks at it, and
redirects it to a url such as /labs/12345/running, or /labs/4567/start.
this is a similar pattern to what is recommended and used in the admin, so
I don't think I am doing something weird here, but I may not know what the
redirect url will look like before I send the request (if I am sending
something like /labs/new, and it returns /labs/12345 for example).
as a hack, I did this:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
def fix_response_for_test(response, re_pattern, replace, count=0,
flags=0):
if hasattr(response, 'redirect_chain'):
url, status_code = response.redirect_chain[-1]
tmp_replaced = re.search(re_pattern, url, flags=flags)
new_url = re.sub(re_pattern, replace, url, count=count,
flags=flags)
# print('redirect - new: %s' % new_url)
response.redirect_chain[-1] = (new_url, status_code)
else:
# Not a followed redirect
url = response.url
scheme, netloc, path, query, fragment = urlsplit(url)
# Prepend the request path to handle relative path redirects.
if not path.startswith('/'):
url = urljoin(response.request['PATH_INFO'], url)
tmp_replaced = re.search(re_pattern, url, flags=flags)
new_url = re.sub(re_pattern, replace, url, count=count,
flags=flags)
# print('no redirected - new: %s' % new_url)
response['Location'] = new_url
return tmp_replaced.group(0)
}}}
}}}
and is run like this:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
session_id = fix_response_for_test(response, UUID_REGEX, '<uuid>')
redirect_url = '/lab/<uuid>/error/'
with self.subTest('%s - response url' % name):
self.assertRedirects(response, redirect_url,
fetch_redirect_response=False, msg_prefix=tmp_msg)
test_session = Sessions.objects.get(session_id=session_id)
# do more testing on the session object to make sure it was
created correctly.
}}}
}}}
The returning the pulled content is nice, but probably not required as I
COULD simply build two tests, one to check the redirect, and another to
test the actual session object.
If I had my druthers, I would love to see something like:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
args_obj=None
self.assertRedirects(response, r'/labs/(?P<foobar>.+)/(.+)',
get_args=args_obj)
# assuming this passes the assertion, args_obj then would ==
# args_obj = {
# 'args': ['list of un-named items'],
# 'kwargs': {dict of kwargs}
}}}
}}}
This coudl also be approached by adding the ability to get this kind of
thing directly from the response object, along the lines of:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
(assuming the request was '/labs/12344/test_page
> my_response.seed_url()
' '//labs//(?P<foobar>.+)//(.+)' # which could then be matched in a
redirect url match.
> my_response.url_params(1)
'test_page'
> my_response.url_params('foobar')
'12344'
}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29098>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 1.11
Severity: Normal | Resolution:
Keywords: unittest redirect | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I'm not really in favor of the additional complexity. In the admin tests,
for example, the problem is solved like this:
{{{
response = <post request to create new object>
new_user = User.objects.get(username='newuser')
self.assertRedirects(response, reverse('admin:auth_user_change',
args=(new_user.pk,)))
}}}
Does that work for you?
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dan J Strohl):
Replying to [comment:1 Tim Graham]:
> I'm not really in favor of the additional complexity. In the admin
tests, for example, the problem is solved like this:
> {{{
> response = <post request to create new object>
> new_user = User.objects.get(username='newuser')
> self.assertRedirects(response, reverse('admin:auth_user_change',
args=(new_user.pk,)))
> }}}
> Does that work for you?
I understand and am generally in agreement with keeping things simple...
While your suggestion would generally work (I think at least, I haven't
tried it), it feels like it would be easier (from the developers pov) to
be able to test if a redirect matched a url pattern, or one of the new url
matching.. (objects?), in addition to simply matching a string. that way
a developer could be as specific (or fuzzy) as they wished. and it would
directly link back to checking the url patterns as well.
The ability to pull out the string is nice, but not really crucial to the
specific feature request.
I would not put this as a critical feature request, but perhaps a bit more
than a "nice to have".
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
It sounds like you'd like `assertRedirects()` to be able to do the
`reverse()` itself. I don't see much advantage to that (complicating
things by allowing `assertRedirects()` to take all the parameters of
`reverse()`). If I misunderstood, can you clarify what API change you're
proposing?
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dan J Strohl):
Replying to [comment:3 Tim Graham]:
> It sounds like you'd like `assertRedirects()` to be able to do the
`reverse()` itself. I don't see much advantage to that (complicating
things by allowing `assertRedirects()` to take all the parameters of
`reverse()`). If I misunderstood, can you clarify what API change you're
proposing?
Ummm, not totally, the reverse() expects enough parameters to be able to
create the url, I am proposing being able to validate without knowing the
specific variables needed to complete the reverse.
basically, at something like the changes below... (all I did was pull out
the last assertion lines and put them into their own method.)
Taking this a bit further, you could also pull out the assertion for the
response codes and make them into their own assertion (which would be
called by the assertRedirects) and/or even put the code to pull the
response codes and response url into a helper function (like
"get_redirected_url(response, fetch_redirect_response=True)" ) further
modularizing it into even simpler pieces, and allowing for more
flexibility in usage.
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
def _assertRedirects(self, response, expected_url, status_code=302,
target_status_code=200, msg_prefix='',
fetch_redirect_response=True):
"""
Assert that a response redirected to a specific URL and that the
redirect URL can be loaded.
Won't work for external links since it uses the test client to do
a
request (use fetch_redirect_response=False to check such links
without
fetching them).
"""
if msg_prefix:
msg_prefix += ": "
if hasattr(response, 'redirect_chain'):
# The request was a followed redirect
self.assertTrue(
len(response.redirect_chain) > 0,
msg_prefix + "Response didn't redirect as expected:
Response code was %d (expected %d)"
% (response.status_code, status_code)
)
self.assertEqual(
response.redirect_chain[0][1], status_code,
msg_prefix + "Initial response didn't redirect as
expected: Response code was %d (expected %d)"
% (response.redirect_chain[0][1], status_code)
)
url, status_code = response.redirect_chain[-1]
scheme, netloc, path, query, fragment = urlsplit(url)
self.assertEqual(
response.status_code, target_status_code,
msg_prefix + "Response didn't redirect as expected: Final
Response code was %d (expected %d)"
% (response.status_code, target_status_code)
)
else:
# Not a followed redirect
self.assertEqual(
response.status_code, status_code,
msg_prefix + "Response didn't redirect as expected:
Response code was %d (expected %d)"
% (response.status_code, status_code)
)
url = response.url
scheme, netloc, path, query, fragment = urlsplit(url)
# Prepend the request path to handle relative path redirects.
if not path.startswith('/'):
url = urljoin(response.request['PATH_INFO'], url)
path = urljoin(response.request['PATH_INFO'], path)
if fetch_redirect_response:
# netloc might be empty, or in cases where Django tests
the
# HTTP scheme, the convention is for netloc to be
'testserver'.
# Trust both as "internal" URLs here.
domain, port = split_domain_port(netloc)
if domain and not validate_host(domain,
settings.ALLOWED_HOSTS):
raise ValueError(
"The test client is unable to fetch remote URLs
(got %s). "
"If the host is served by Django, add '%s' to
ALLOWED_HOSTS. "
"Otherwise, use assertRedirects(...,
fetch_redirect_response=False)."
% (url, domain)
)
redirect_response = response.client.get(path,
QueryDict(query), secure=(scheme == 'https'))
# Get the redirection page, using the same client that was
used
# to obtain the original response.
self.assertEqual(
redirect_response.status_code, target_status_code,
msg_prefix + "Couldn't retrieve redirection page '%s':
response code was %d (expected %d)"
% (path, redirect_response.status_code,
target_status_code)
)
return url
def assertRedirects(self, response, expected_url, status_code=302,
target_status_code=200, msg_prefix='',
fetch_redirect_response=True):
url = self._assertRedirects(
response,
expected_url,
status_code=status_code,
target_status_code=target_status_code,
msg_prefix=msg_prefix,
fetch_redirect_response=fetch_redirect_response)
self.assertEqual(
url, expected_url,
msg_prefix + "Response redirected to '%s', expected '%s'" %
(url, expected_url)
)
def assertRedirectsRegex(self, response, expected_url_regex,
status_code=302,
target_status_code=200, msg_prefix='',
fetch_redirect_response=True):
url = self._assertRedirects(
response,
expected_url,
status_code=status_code,
target_status_code=target_status_code,
msg_prefix=msg_prefix,
fetch_redirect_response=fetch_redirect_response)
self.assertRegex(
url, expected_url_regex,
msg_prefix + "Response redirected to '%s', expected '%s'" %
(url, expected_url)
)
}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:4>
Django
-----------------------------------+------------------------------------
Reporter: Dan J Strohl | Owner: nobody
Component: Testing framework | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Cleanup/optimization => New feature
* stage: Unreviewed => Accepted
Old description:
New description:
response['Location'] = new_url
return tmp_replaced.group(0)
}}}
}}}
This could also be approached by adding the ability to get this kind of
thing directly from the response object, along the lines of:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
(assuming the request was '/labs/12344/test_page
> my_response.seed_url()
' '//labs//(?P<foobar>.+)//(.+)' # which could then be matched in a
redirect url match.
> my_response.url_params(1)
'test_page'
> my_response.url_params('foobar')
'12344'
}}}
}}}
--
Comment:
I don't like it, but I suppose it would be consistent with other unittest
assertions.
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:5>
Django
-----------------------------------+-------------------------------------
Reporter: Dan J Strohl | Owner: Dan Davis
Type: New feature | Status: assigned
Component: Testing framework | Version: 2.1
Severity: Normal | Resolution:
Keywords: unittest redirect | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* owner: nobody => Dan Davis
* status: new => assigned
* version: 1.11 => 2.1
Comment:
I'll take this one. Seems straight-forward enough. Not sure why it
should be done on 1.11, because it doesn't seem severe enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:6>
Django
-----------------------------------+-------------------------------------
Reporter: Dan J Strohl | Owner: Dan Davis
Type: New feature | Status: assigned
Component: Testing framework | Version: 2.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
The version field simply reflects when the ticket was reported. All new
features target the development branch of Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:7>
Django
-----------------------------------+------------------------------------
Reporter: Dan J Strohl | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* cc: Dan Davis (added)
* owner: Dan Davis => (none)
* status: assigned => new
Comment:
Someone else can work on this issue if they have time.
--
Ticket URL: <https://code.djangoproject.com/ticket/29098#comment:8>