#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)
Change History (14)
comment:1 by , 8 years ago
Summary: | AssertRedirects expects ordered query parameters → Make SimpleTestCase.assertRedirects() URL comparison ignore ordering of query parameters |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 7 years ago
Attachment: | assertRedirects.patch added |
---|
Patch adding normalization to the url/expected_url prior to self.assertEqual
comment:3 by , 7 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 , 7 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 , 7 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Pull request: https://github.com/django/django/pull/9480
comment:6 by , 7 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 , 7 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 , 7 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 , 7 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 , 6 years ago
Patch needs improvement: | unset |
---|---|
Version: | 1.10 → master |
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 , 6 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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?