Opened 6 years ago

Last modified 5 years ago

#29098 new New feature

Add SimpleTestCase.assertRedirectsRegex()

Reported by: Dan J Strohl Owned by:
Component: Testing framework Version: 2.1
Severity: Normal Keywords: unittest redirect
Cc: Dan Davis Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

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:

Code highlighting: 

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:

Code highlighting: 

      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:

Code highlighting: 

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 could also be approached by adding the ability to get this kind of thing directly from the response object, along the lines of:

Code highlighting: 

(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'

Change History (8)

comment:1 by Tim Graham, 6 years ago

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?

in reply to:  1 comment:2 by Dan J Strohl, 6 years ago

Replying to 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".

comment:3 by Tim Graham, 6 years ago

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?

in reply to:  3 comment:4 by Dan J Strohl, 6 years ago

Replying to 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.

Code highlighting: 


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)
        )

comment:5 by Tim Graham, 6 years ago

Description: modified (diff)
Summary: Allow assertRedirects to handle regex matches. → Add SimpleTestCase.assertRedirectsRegex()
Triage Stage: Unreviewed → Accepted
Type: Cleanup/optimization → New feature

I don't like it, but I suppose it would be consistent with other unittest assertions.

comment:6 by Dan Davis, 5 years ago

Owner: changed from nobody to Dan Davis
Status: new → assigned
Version: 1.11 → 2.1

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.

comment:7 by Tim Graham, 5 years ago

The version field simply reflects when the ticket was reported. All new features target the development branch of Django.

comment:8 by Dan Davis, 5 years ago

Cc: Dan Davis added
Owner: Dan Davis removed
Status: assigned → new

Someone else can work on this issue if they have time.

Note: See TracTickets for help on using tickets.
Back to Top