Opened 8 years ago

Closed 7 years ago

#5538 closed (fixed)

assertRedirects should assume http://testserver if it isn't provided

Reported by: russellm Owned by: anonymous
Component: Testing framework Version: master
Severity: Keywords: assertRedirects testserver
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by russellm)

[6164] fixed #987, a problem with redirects allowing relative URIs. The fix is correct, but it makes the testing API slightly unwieldy; all calls to assertRedirects() now require that 'http://testserver' be part of the asserted URL.

The vast majority of redirection tests will be internal to the site, so requiring the full dummy host prefix only serves to muddy the tests. This is made worse by the fact that urlresolvers.reverse() doesn't include the testserver component, so assertRedirects(reverse('some_name')) doesn't work.

assertRedirects should be modified to inspect the test URL; if no ':' doesn't occur in the test url, 'http://testserver' should be prepended for test purposes. i.e.,

assertRedirects('/some/url') is equivalent to assertRedirects('http://testserver/some/url')

assertRedirects('http://otherserver/some/url') would be untouched

Attachments (1)

testcases.py (8.5 KB) - added by pat.m.boyd@… 7 years ago.
patch

Download all attachments as: .zip

Change History (6)

comment:1 Changed 8 years ago by russellm

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Accepted to Design decision needed

Uh, I don't think I changed the requirement on assertRedirect tests, since the test server doesn't set HTTP_HOST or SERVER_NAME

see http://code.djangoproject.com/changeset/6164#file4

comment:3 Changed 8 years ago by russellm

Ah - Looks like I mispointed the finger. It wasn't [6164] - it was [6166], fixing #4986. The test were updated in [6169]. I have a whole lot of tests that are similarly affected.

Changed 7 years ago by pat.m.boyd@…

patch

comment:4 Changed 7 years ago by patrickb

  • Has patch set
  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

I attached a solution to only match parts of the url that were defined in the expected_url. It fixes our tests but I'm not sure if its good/clear/readable/up to django standards.

comment:5 Changed 7 years ago by mtredinnick

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

Faced with having to modify a couple of dozen tests in [6662], I fixed this in [6661]. Handles both relative and absolute URLs, as well as making absolute URLs from a passed in host parameter in case that's easier sometimes (one case in the tests uses django.testserver as the host, for example).

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