Opened 12 years ago
Closed 12 years ago
#19462 closed Cleanup/optimization (fixed)
Make assertQSEqual error out if no ordering defined
Reported by: | Anssi Kääriäinen | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
An unordered queryset isn't guaranteed to return the values in any order. So, using assertQuerysetEqual comparing more than one value and an unordered qs with the ordered flag set is likely a programming error.
As seen from the linked patch this error is fairly common.
In practice databases usually return the values in insert order, so the error does not surface often. Still, this can be a source of heisenbugs as background database maintenance jobs can alter the ordering of the results.
The linked patch contains two commits, one for making assertQuerysetEqual fail if the ordering isn't well defined, and one to fix all the tests in Django where the ordering isn't well defined.
Patch here: https://github.com/akaariai/django/compare/assertqs_ordering
I guess we need a short mention in release notes about this?
Change History (6)
comment:1 by , 12 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
Version: | 1.4 → master |
comment:2 by , 12 years ago
I have updated the patch (at https://github.com/akaariai/django/compare/assertqs_ordering). It now contains docs, tests for assertQuerysetEqual and in addition the assertQSEqual raises ValueError on undefined order instead of doing a self.fail(). The last change is from discussions at IRC.
I think this is now ready for commit. Review still welcome (especially docs review).
comment:3 by , 12 years ago
As far as code is concerned, this is indeed RFC. I've seen a typo ("is seens as"), but I don't count for a language reviewer :-)
comment:4 by , 12 years ago
Needs tests: | unset |
---|
comment:5 by , 12 years ago
I bet the docs changes contain a couple of things which would make my English teacher cry...
Still, I think I will commit this one with the typo fix, I have too many tickets in "minor polish needed, else ready" state...
comment:6 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
It seems that
assertQuerysetEqual
is not tested (in regressiontests/test_utils). Could you add some basic tests?