Opened 4 years ago

Closed 4 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: master
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 Changed 4 years ago by Claude Paroz

Needs tests: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.4master

It seems that assertQuerysetEqual is not tested (in regressiontests/test_utils). Could you add some basic tests?

comment:2 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Claude Paroz

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 Changed 4 years ago by Claude Paroz

Needs tests: unset

comment:5 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In 088d3bc2f84b6b68fee7e5de053b58049bd110e7:

Fixed #19462 -- Made assertQuerysetEqual detect undefined ordering

If there are more than one values to compare against and the qs isn't
ordered then assertQuerysetEqual will raise a ValueError.

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