Opened 3 years ago

Closed 3 years ago

#19462 closed Cleanup/optimization (fixed)

Make assertQSEqual error out if no ordering defined

Reported by: akaariai 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 3 years ago by claudep

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.4 to master

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

comment:2 Changed 3 years ago by akaariai

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 3 years ago by claudep

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 3 years ago by claudep

  • Needs tests unset

comment:5 Changed 3 years ago by akaariai

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

  • Resolution set to fixed
  • Status changed from new to closed

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