Opened 11 years ago

Closed 11 years ago

#20387 closed Bug (fixed)

Two tests for admin views fail under Oracle

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: contrib.admin Version: dev
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aymeric Augustin)

http://ci.djangoproject.com/job/Django%20Oracle/88/database=oracle,python=python2.7/testReport/admin_views.tests/

Displaying the user or the group change view does one less database query than expected.

Attachments (1)

20387.patch (1.7 KB ) - added by Aymeric Augustin 11 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Aymeric Augustin, 11 years ago

Description: modified (diff)

comment:2 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

This is related to the new transaction management.

Compared to SQLite, Oracle is missing a RELEASE SAVEPOINT statement.

comment:3 by Aymeric Augustin, 11 years ago

Oracle doesn't provide RELEASE SAVEPOINT. (I hope you were sitting down when you read that.)

This deficiency is reflected here: https://github.com/django/django/commit/a4f1e48ddbc464ed0ccb9c305db721845db91d6b#L0R319.

Possible solutions include:

  • Not counting savepoint-related statements in assertNumQueries — there's some merit in this idea, as it would minimize test breakage in the 1.6 release, but it goes against the principle of least surprise.
  • Executing a comment eg. query.execute("-- RELEASE SAVEPOINT ...") under Oracle to keep the query counts identical on all backends,
  • Adding an implements_release_savepoint feature, and make the expected query count depend on that feature,
  • Simply make it depend on connection.vendor == 'oracle'.

Thoughts?

I'm attaching a patch implementing the last solution, because I'm lazy.

by Aymeric Augustin, 11 years ago

Attachment: 20387.patch added

comment:4 by Anssi Kääriäinen, 11 years ago

Last one seems good enough to me. We can switch the strategy later on if there is need for that...

comment:5 by Shai Berger, 11 years ago

Last one means every assertNumQueries() where a nested transaction is involved (which is almost all of them) now needs to test for vendor==oracle. I think the best solution, if possible, is to increment the query count on the Oracle backend without executing anything.

The above notwithstanding, the patch has a with self.assertNumQueries(9): in line 3615 that should be with self.assertNumQueries(expected_queries):

comment:6 by Anssi Kääriäinen, 11 years ago

Hmmh, yes, adding a dummy query to connection.queries without executing it seems like an excellent solution to me. The query could be something like "-- dummy RELEASE SAVEPOINT [not executed]". Maybe we should even add a connection._fake_execute() for this purpose so that Oracle doesn't need to know anything about connection.queries implementation?

comment:7 by Aymeric Augustin, 11 years ago

Actually I pushed the patch in a4dec43b520fa51bf7a949576b5767242c74c982 — the post-commit hook has been flaky lately.

I filed #20420 to track the ideas in the two last comments.

comment:8 by Aymeric Augustin, 11 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top