Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#27398 closed Cleanup/optimization (fixed)

Make SimpleTestCase.assertRedirects() URL comparison ignore ordering of query parameters

Reported by: Cédric Codet Owned by: Jan Pieter Waagmeester
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When checking for a url redirection in tests with assertRedirects, the order of redirection url query parameters matters (since it is matched with expected_url string argument).

Example: if the expected url is example.com/?foo=1&bar=2, example.com/?bar=2&foo=1 is not matched (while example.com/?foo=1&bar=2 is)

This is an issue since Django does not enforce query parameters ordering itself

Attachments (1)

assertRedirects.patch (3.2 KB ) - added by Jan Pieter Waagmeester 6 years ago.
Patch adding normalization to the url/expected_url prior to self.assertEqual

Download all attachments as: .zip

Change History (14)

comment:1 by Tim Graham, 7 years ago

Summary: AssertRedirects expects ordered query parametersMake SimpleTestCase.assertRedirects() URL comparison ignore ordering of query parameters
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Seems useful as Django's test suite has a workaround for this issue. I'm not sure if we need a way to toggle the behavior -- always ignoring ordering seems fine unless anyone can think of a use case where it would be problematic?

comment:2 by favll, 7 years ago

Owner: changed from nobody to favll
Status: newassigned

by Jan Pieter Waagmeester, 6 years ago

Attachment: assertRedirects.patch added

Patch adding normalization to the url/expected_url prior to self.assertEqual

comment:3 by Jan Pieter Waagmeester, 6 years ago

Just attached a patch which fixes this issue, if the general direction of this fix is considered the way to go, I am willing to open a pull request on Github.

Do we need the strict_query_order param to provide a way to fall back to current behavior?

comment:4 by Claude Paroz, 6 years ago

Please make the PR.
I wouldn't keep the strict_query_order param, unless we can find a reference stating that param ordering in query string is significant and kept.

comment:5 by Jan Pieter Waagmeester, 6 years ago

Has patch: set
Owner: changed from favll to Jan Pieter Waagmeester

comment:6 by Simon Charette, 6 years ago

I'm not sure if this is relevant here but Django makes a distinction between ?foo=1&foo=2 and ?foo=2&foo=1 so we might have to make the strict_query_order parameter default to True?

comment:7 by Jan Pieter Waagmeester, 6 years ago

I just pushed a commit allowing multiple values for a key in the redirect_view test view. Indeed this allows reordering foo=1&foo=2 to foo=2&foo=1 in assertRedirects without failures.

There are currently no tests to test that.

There is also AuthViewTestCase.assertURLEqual() which compares two QueryDict objects, which ensures order for multiple values is checked.

We can move the assertUrlRedirects method to SimpleTestCase and use that method (or a similar implementation) to do the final url equality assertion in SimpleTestCase.assertRedirects().

comment:8 by Carlton Gibson, 6 years ago

Patch needs improvement: set

Hi Jan,

Thanks for your effort here!

I think the points you make in your last comment are all correct.

The first step is adding the test cases to cover the foo=1&foo=2 vs foo=2&foo=1 example. These are indeed different URLs.

You have foo=1&bar=1 vs bar=1&foo=1 already. Are there other examples we need to cover?

Excepting the multi-value case, I don't see order being important. (Anyone?)

I think your proposal to move and reuse assertURLEqual() method seems very sensible. (Going via QueryDict should be more reliable that trying to normalise the URL by-hand.)

comment:9 by Jan Pieter Waagmeester, 6 years ago

I just pushed a commit splitting of assertURLRedirects(), and included some extra tests.

I tried using QueryDict, but they preserve their creation order when .urlencode() is called and thus do not allow re-creating the url. I like to do that because it simplifies the output, but if you still think the way it was done in the implementation I removed, I'll update my implementation to look more like that one.

Do we need to add tests checking the message produced if the assertion fails?

comment:10 by Jan Pieter Waagmeester, 6 years ago

Patch needs improvement: unset
Version: 1.10master

I just pushed a new commit:

  • documentation for the new assertion to docs/topics/testing/tools.txt
  • Wrapped some docsgtrings to 79 chars.
  • Added a changelog entry

comment:11 by Carlton Gibson, 6 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In 24959e48:

Fixed #27398 -- Added an assertion to compare URLs, ignoring the order of their query strings.

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

In 5d98d53:

Refs #27398 -- Simplified some tests with assertRedirects().

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