Code

Opened 5 years ago

Closed 5 years ago

#10381 closed (fixed)

Test failure caused by r9921

Reported by: mtredinnick Owned by: russellm
Component: Uncategorized Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This looks minor and I don't want to interrupt my flow to dive deep now. I'm seeing on test failure (both in the full suite and in isolation):

.-(malcolm@lancre 15:10:35) ~/BleedingEdge/Django/django.git/tests
`--> ./runtests.py --settings=settings fixtures
======================================================================
FAIL: Doctest: modeltests.fixtures.models.__test__.API_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for modeltests.fixtures.models.__test__.API_TESTS
  File "/home/malcolm/BleedingEdge/Django/django.git/tests/modeltests/fixtures/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/malcolm/BleedingEdge/Django/django.git/tests/modeltests/fixtures/models.py", line ?, in modeltests.fixtures.models.__test__.API_TESTS
Failed example:
    management.call_command('dumpdata', 'fixtures.Category', 'sites', format='json')
Expected:
    [{"pk": 1, "model": "fixtures.category", "fields": {"description": "Latest news stories", "title": "News Stories"}}, {"pk": 1, "model": "sites.site", "fields": {"domain": "example.com", "name": "example.com"}}]
Got:
    [{"pk": 1, "model": "sites.site", "fields": {"domain": "example.com", "name": "example.com"}}, {"pk": 1, "model": "fixtures.category", "fields": {"description": "Latest news stories", "title": "News Stories"}}]


----------------------------------------------------------------------
Ran 2 tests in 2.773s

FAILED (failures=1)

Git-bisect narrows it down to r9921 (ticket #5610), which seems quite believable.

Attachments (1)

10381-r9921.diff (1.0 KB) - added by russellm 5 years ago.
Patch to enforce predictable ordering in dumpdata output

Download all attachments as: .zip

Change History (5)

Changed 5 years ago by russellm

Patch to enforce predictable ordering in dumpdata output

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

This appears to be a dictionary ordering problem. I can't reproduce it myself, but I suspect that the attached patch should fix the problem.

comment:2 Changed 5 years ago by mtredinnick

Oh, sorry ... I was too blind to notice it was just a permuted order. :-(

Is there some reason the order needs to be preserved. What I mean is: SortedDict isn't free to use. Should we just compare the output in the test using == instead of doing a repr comparison to avoid having to insert something in the core code?

The patch does make the problem go away.

comment:3 Changed 5 years ago by russellm

Granted, SortedDict isn't free, but the overhead isn't particularly expensive in this particular case. Worst case, you have a dictionary with as many keys as you have apps. It's also transient storage - it only exists for the duration of the dumpdata command call. This isn't an area where we have to be especially paranoid about efficiency.

There is also a minor advantage to maintaining the order (aside from making the tests work) - the apps will be serialized in the order that they are specified on the command line.

Just replacing the comparison with a == isn't a simple solution here - the output is a serialized JSON string, not just a list of dicts; also, the outer structure is a list, so the order is significant on comparison (not in a particularly meaningful way in this case, but it would still defeat any simple parsed JSON comparison)

One alternate approach would be to serialize apps in the same order the occur in the INSTALLED_APPS setting. This would yield a predictable iteration order at the cost of a marginally more CPU intensive approach to developing the iteration list. Personally, I'm comfortable with the cost of the SortedDict approach in this case, but I'm happy to use INSTALLED_APPS order too. Let me know which one you would prefer and I'll commit.

comment:4 Changed 5 years ago by mtredinnick

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

(In [9964]) Fixed #10381 -- Fixed some a machine-dependent test failure after r9921.

The patch is from Russell, but I'm applying it because I want my tests to pass
again (and he doesn't see the failure).

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.