Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#19489 closed Cleanup/optimization (fixed)

assertRedirects host parameter not in the doc

Reported by: mrknacky@… Owned by: gajimenezmaggiora
Component: Documentation Version: 1.4
Severity: Normal Keywords: doc assertRedirects
Cc: timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I was searching in the doc and in the web for changing the http_host of the last assertEqual test of assertRedirects and in the doc host parameter doesn't exist.

In order to find it I had to find it in the source code

Change History (10)

comment:1 by Tim Graham, 11 years ago

I'm not sure the use case for this, could you given an example? The docstring for assertRedirects says "Note that assertRedirects won't work for external links since it uses TestClient to do a request."

The assertion in the Django test suite:

self.assertRedirects(response, '/test_client/get_view/', host=host)

Could just as easily be rewritten:

self.assertRedirects(response, 'http://%s/test_client/get_view/' % host)

This was added in commit 30848dfe34bb027c40ae1902623bd2fb675f6424.

comment:2 by Russell Keith-Magee, 11 years ago

Triage Stage: UnreviewedAccepted

Well, presumable the use case is documenting a feature that is available. This isn't asking for a new feature; it's documenting an existing one.

comment:3 by Tim Graham, 11 years ago

I meant what is the use case for using a different host in assertRedirects (since assertRedirects doesn't work for external links). I'd like to be able to document why someone would want to use the argument.

comment:4 by gajimenezmaggiora, 11 years ago

Owner: changed from nobody to gajimenezmaggiora
Status: newassigned

comment:6 by gajimenezmaggiora, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Tim Graham, 11 years ago

Cc: timograham@… added
Has patch: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Please don't mark your own patch as RFC.

The patch looks ok, but I'm still not clear about how/why I would actually use the host parameter. As I noted earlier, the docstring says "Note that assertRedirects won't work for external links since it uses TestClient to do a request." Indeed, if I try to use assertRedirects on a view that returns something like HttpResponseRedirect("http://www.djangoproject.com/download/"), the test client will attempt to fetch /download/ of the app that's being tested, not of djangoproject.com.

I see the test that was added when this was originally committed works by instantiating the TestClient like this: Client(HTTP_HOST=host), where host is 'django.testserver'. I'm all for documenting this if there's some use case, but as is, I think this will cause confusion.

comment:8 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In e3c7af18a307f9d020eb798bdad6cc63ab22dac9:

Fixed #19489 -- Documented host parameter of assertRedirects().

Thanks mrknacky at gmail.com for the report and gajimenezmaggiora
for the draft patch.

comment:9 by Tim Graham <timograham@…>, 10 years ago

In c45fcd278b48b066ef03765c82005ba96183db09:

[1.7.x] Fixed #19489 -- Documented host parameter of assertRedirects().

Thanks mrknacky at gmail.com for the report and gajimenezmaggiora
for the draft patch.

Backport of e3c7af18a3 from master

comment:10 by Tim Graham <timograham@…>, 10 years ago

In 735ece0b89b400b95c5e38ae640a7c3fe860c377:

[1.6.x] Fixed #19489 -- Documented host parameter of assertRedirects().

Thanks mrknacky at gmail.com for the report and gajimenezmaggiora
for the draft patch.

Backport of e3c7af18a3 from master

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