Opened 8 years ago

Closed 6 years ago

#4476 closed (fixed)

Modify test client to follow redirect chains

Reported by: Chris Moffitt <chris.moffitt@…> Owned by: telenieko
Component: Testing framework Version: master
Severity: Keywords:
Cc: kbussell@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by russellm)

When using assertRedirects, I find myself doing an assert redirects, then wanting to validate the text where I'm redirecting. The process seems a little clunky and an assertRedirectContains would be useful.

Current process:

self.assertRedirects(response, '/confirm/', status_code=302, target_status_code=200)
response = self.client.get('/confirm/')
self.assertContains(response, "Total = $54.50", count=1, status_code=200)

It would be nice to have something like:

self.assertRedirectContains(response, "Total = $54.50", count=1, status_code=302, target_status_code=200)

I'm assuming there's no other shortcut I'm missing.

* UPDATE (from russellm) *

Rather that assertRedirectContains, I'm favouring a modification to the test client that will follow redirects. Details in the comment below

Attachments (4)

4476.diff (5.5 KB) - added by telenieko 7 years ago.
Patch with docs+tests
4476_follow_redirects.diff (14.3 KB) - added by keithb 7 years ago.
Another version of the fix, since I already did the work
4476_rev7412.diff (5.6 KB) - added by telenieko 7 years ago.
Updated patch, used my version for being shorter ;))
4476_mixed.diff (12.5 KB) - added by telenieko 6 years ago.
New patch, comments below.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by John Shaffer <jshaffer2112@…>

  • Component changed from Uncategorized to Unit test system
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from jacob to adrian
  • Patch needs improvement unset

comment:2 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 follow-up: Changed 8 years ago by russellm

  • Description modified (diff)
  • Summary changed from Proposal for assertRedirectContains to Modify test client to follow redirect chains
  • Triage Stage changed from Design decision needed to Accepted

I'm not a fan of assertRedirectContains, because it sounds like start of aggregating groups of tests together. It works for a single redirect, but then you have a page that redirects to a redirect, so you need to add 'assertRedirectRedirectContains', and then the madness starts :-). So, -1 to the literal idea.

However, I'm going to mark the ticket is as accepted and change the summary, because the general problem is valid.

Rather than assertRedirectContains, I'm going to suggest that the test Client should be modified to do what a normal web client will do - follow redirect trails. So, if you had a site with:

/first -> redirects to /second
/second -> redirects to /final
/final -> a page of content

response = client.get('/first', follow=True)

you would get back the content at '/final', with a response code of 200. The response should also have some extra meta-data to indicate the paths that were visited along the way. There may also be a need to modify the assertions to allow for checking that a redirect occurred; e.g.,

assertContains(response, "content of /final", count=1, status_code=200, redirected=True)

However, I'll leave the details of any assertion changes as a detail for the implementer.

comment:4 Changed 8 years ago by frasern

  • Owner changed from nobody to frasern
  • Status changed from new to assigned

comment:5 Changed 7 years ago by frasern

  • Owner changed from frasern to nobody
  • Status changed from assigned to new

comment:6 Changed 7 years ago by keithb

  • Owner changed from nobody to keithb

comment:7 in reply to: ↑ 3 Changed 7 years ago by telenieko

Replying to russellm:

you would get back the content at '/final', with a response code of 200. The response should also have some extra meta-data to indicate the paths that were visited along the way. There may also be a need to modify the assertions to allow for checking that a redirect occurred; e.g.,

What about returning an array of the different response objects?

responses = client.get(..., follow=True)

If follow=True, then always return an array with all the responses got following the redirects.

Changed 7 years ago by telenieko

Patch with docs+tests

comment:8 Changed 7 years ago by telenieko

  • Has patch set
  • Owner changed from keithb to telenieko
  • Status changed from new to assigned

Attached patch with docs, tests and the code itself.
It works for .post() and .get()

Hope it's fine for all of you ;)

comment:9 Changed 7 years ago by telenieko

keithb, sorry I just noticed you assigned the ticket to yourself. Hope I didn't make you waste too much time! :)

Changed 7 years ago by keithb

Another version of the fix, since I already did the work

comment:10 Changed 7 years ago by keithb

No worries. Since I already did the work for the fix, and my solution is slightly different, I figured I'd post it anyway.

The main differences in my version:

  • instead of returning multiple responses, a redirect_chain attribute is added to the response which holds the intermediate paths and status codes.
  • follow is True by default
  • the redirect handling prevents against loops in the chain.
  • assertRedirects takes an extra parameter if you want to check intermediate redirect steps, but defaults to checking the last

comment:11 Changed 7 years ago by keithb

  • Cc kbussell@… added

Changed 7 years ago by telenieko

Updated patch, used my version for being shorter ;))

comment:12 Changed 7 years ago by telenieko

Just attached an updated patch, I took two response codes from keithb patch.

comment:13 Changed 7 years ago by telenieko

The docs-refactor broke the patch, always up-to-date patch here in colours and raw

comment:14 Changed 6 years ago by miohtama

Any hope this could land to Django 1.1?

comment:15 follow-up: Changed 6 years ago by russellm

@telenieko: What was the reasoning for returning a list of responses? Personally, I prefer keithb's approach of returning the final response, but keeping the chain available as an attribute on the response.

Also - keithb's patch catches some important test cases, such as the infinite loop redirect and redirect to 404.

Changed 6 years ago by telenieko

New patch, comments below.

comment:16 in reply to: ↑ 15 Changed 6 years ago by telenieko

Replying to russellm:

@telenieko: What was the reasoning for returning a list of responses? Personally, I prefer keithb's approach of returning the final response, but keeping the chain available as an attribute on the response.

I have no idea ;

I attached a new patch taking keithb's approach (return the final response). And taking care of backwards compatibility. Hope it's ok now.

comment:17 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [9911]) Fixed #4476 -- Added a follow option to the test client request methods. This implements browser-like behavior for the test client, following redirect chains when a 30X response is received. Thanks to Marc Fargas and Keith Bussell for their work on this.

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