Opened 4 years ago

Closed 2 years ago

Last modified 2 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 Changed 4 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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 Changed 4 years ago by Russell Keith-Magee

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 Changed 4 years ago by Tim Graham

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 Changed 3 years ago by gajimenezmaggiora

Owner: changed from nobody to gajimenezmaggiora
Status: newassigned

comment:5 Changed 3 years ago by gajimenezmaggiora

comment:6 Changed 3 years ago by gajimenezmaggiora

Triage Stage: AcceptedReady for checkin

comment:7 Changed 3 years ago by Tim Graham

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 Changed 2 years ago by Tim Graham <timograham@…>

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 Changed 2 years ago by Tim Graham <timograham@…>

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 Changed 2 years ago by Tim Graham <timograham@…>

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