1159337 - Occasional incorrect ordering of feature.children during test of PUT to view_feature

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: BrowserCompat, defect)

Description

[John Whitlock [:jwhitlock]]

What did you do?
================
About 5% of the time, the test 'test_post_add_second_subfeature' in webplatform/tests/test_view_serializers.py will fail.

What happened?
==============
The test fails because the order of two child features is swapped.    The test has always succeeded on the second run.

What should have happened?
==========================
The expected order of child features is creation order - the existing child feature, then the newly added child feature

Is there anything else we should know?
======================================
Order should be determined by the django-mptt 3rd-party app.  This could be an issue with django-mptt 0.7.1, or with the code that handles updating of mptt fields after an update (see webplatformcompat/view_serializers.py, FeatureExtra.save).

This happens locally and in TravisCI.  It seems to happen in the Python 3.4 tests, but that could be faulty memory.  The test has succeeded on the second run, which had made debugging difficult.

Comment 1

[John Whitlock [:jwhitlock]]

I just got the bug under Python 2.7, so it's not a 3.4 issue.

Comment 2

[John Whitlock [:jwhitlock]]

I've added some code to dig in further (https://github.com/jwhitlock/web-platform-compat/commit/2f06ac5474526b376c5d0f51952370060b82feac) and was rewarded with a bug report:

======================================================================
Unexpected failure in test_post_add_second_subfeature.
Report at:
 https://bugzilla.mozilla.org/show_bug.cgi?id=1159337
parent: id=430, lft=1, rght=4, tree_id=1, level=0, parent=None
parent from DB: id=430, lft=1, rght=6, tree_id=1, level=0, parent=None
sf1: id=431, lft=2, rght=3, tree_id=1, level=1, parent=feature
sf2: id=432, lft=4, rght=5, tree_id=1, level=1, parent=feature
======================================================================

There is definitely an issue.  The parent rght field should be 6, and is when read directly from the database, but is the old value of 4 in the subject code.  More investigation is needed, starting with database traces.

Comment 3

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/web-platform-compat

https://github.com/mozilla/web-platform-compat/commit/8be53a3d395639dbcde5c4badca24246e24008cc
bug 1159337 - Debug occasional test failure

When the test test_post_add_second_subfeature results in an unexpected
feature order, print a bunch of debug data, including the MPTT state and
database queries, before failing the test. This debug code does not run
under normal API calls, only in the test.

https://github.com/mozilla/web-platform-compat/commit/c79b02f88b3bb6c8ab594d0ec12ed90a05903b06
Merge pull request #42 from jwhitlock/1159337_feature_ordering_bug

bug 1159337 - Debug occasional test failure

Comment 4

[John Whitlock [:jwhitlock]]

New instance, but DB queries didn't work.  The disconnect between the instance's parent.rght and the database's parent.rght appears again.

======================================================================
Unexpected failure in test_post_add_second_subfeature.
Report at:
 https://bugzilla.mozilla.org/show_bug.cgi?id=1159337
DB queries:
Features:
parent: id=340, lft=1, rght=4, tree_id=1, level=0, parent=None
parent from DB: id=340, lft=1, rght=6, tree_id=1, level=0, parent=None
sf1: id=341, lft=2, rght=3, tree_id=1, level=1, parent=feature
sf2: id=342, lft=4, rght=5, tree_id=1, level=1, parent=feature
======================================================================

Comment 5

[John Whitlock [:jwhitlock]]

This occurred again intermittently while upgrading dependencies.  The parent.rght disconnect doesn't seem to be the issue - that's a result of Django caching instance values.  Mysterious.

Comment 6

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mdn/browsercompat

https://github.com/mdn/browsercompat/commit/303b1a1565de14520e4f645962e51ef555651ae4
bug 1159337 - Add DB queries for test failure

This code will capture the database queries, so that the queries for a
failing test can be compared to the passing test.

https://github.com/mdn/browsercompat/commit/f40af6cd42e5814ddd3e4c20aad8d7dbb534ac9f
Merge pull request #52 from mdn/debug_incorrect_order_1159337

bug 1159337 - Add DB queries for test failure

Comment 7

[John Whitlock [:jwhitlock]]

Attachment: bug_report.txt

Failure with DB queries

Comment 8

[MDN Team (:mdn-dev)]

Commit pushed to master at https://github.com/mdn/browsercompat

https://github.com/mdn/browsercompat/commit/671344f4673bd4ac97a70cd0509274344dd5c346
fix bug 1159337 - Use TreeManager for Feature

Feature should be using a manager derived from django-mptt's
TreeManager, not the Django-standard manager.  Feature.children will now
return child features in tree order.