Opened 3 years ago

Closed 14 months ago

Last modified 14 months 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 3 years ago by timo

  • 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 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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

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

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

comment:5 Changed 2 years ago by gajimenezmaggiora

comment:6 Changed 2 years ago by gajimenezmaggiora

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 2 years ago by timo

  • Cc timograham@… added
  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 14 months ago by Tim Graham <timograham@…>

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

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 14 months 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 14 months 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