Opened 22 months ago

Closed 2 months ago

Last modified 2 months 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: master
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 8 months ago.
Patch adding normalization to the url/expected_url prior to self.assertEqual

Download all attachments as: .zip

Change History (14)

comment:1 Changed 22 months ago by Tim Graham

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 Changed 22 months ago by favll

Owner: changed from nobody to favll
Status: newassigned

Changed 8 months ago by Jan Pieter Waagmeester

Attachment: assertRedirects.patch added

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

comment:3 Changed 8 months ago by Jan Pieter Waagmeester

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 Changed 8 months ago by Claude Paroz

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 Changed 8 months ago by Jan Pieter Waagmeester

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

comment:6 Changed 8 months ago by Simon Charette

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 Changed 8 months ago by Jan Pieter Waagmeester

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 Changed 7 months ago by Carlton Gibson

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 Changed 6 months ago by Jan Pieter Waagmeester

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 Changed 3 months ago by Jan Pieter Waagmeester

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 Changed 2 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:12 Changed 2 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 24959e48:

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

comment:13 Changed 2 months ago by Tim Graham <timograham@…>

In 5d98d53:

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

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