Opened 13 years ago
Closed 12 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 )
Displaying the user or the group change view does one less database query than expected.
Attachments (1)
Change History (9)
comment:1 by , 13 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:3 by , 13 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_savepointfeature, 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 , 13 years ago
| Attachment: | 20387.patch added |
|---|
comment:4 by , 13 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 , 12 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 , 12 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 , 12 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 , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
This is related to the new transaction management.
Compared to SQLite, Oracle is missing a
RELEASE SAVEPOINTstatement.