Opened 16 months ago

Closed 13 months ago

Last modified 13 months ago

#22491 closed Cleanup/optimization (fixed)

TestCase + select_for_update()

Reported by: andreas_pelme Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since 1.6.3 and the select_for_update() changes, TransactionManagementError is raised when run in autocommit mode.

However, in TestCase subclasses select_for_update() with missing transaction.atomic() blocks are not detected, since the test case itself runs in an atomic block.

This means that select_for_update() errors will be silent in most test suites, and then show up in production.

I think a lot of pain could be saved if it was possible to detect these cases and raise an exception in the tests to point out problems. I am not sure how (if) that could be accomplished, but if it was possible, I think it would be a big win. I would be happy to work on a patch if you think this would be a feasible idea.

Change History (7)

comment:1 Changed 16 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

That looks hard to implement. How would you tell an atomic block created by a TestCase apart from an atomic block in application code exercised by a TransactionTestCase?

If you can propose an implementation that makes sense, yes, let's do it. Otherwise we'll have to document that code involving select_for_update should be tested in a TransactionTestCase.

comment:2 Changed 16 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

Accepting at least the documentation route if it's not feasible.

comment:3 Changed 13 months ago by mardini

  • Component changed from Database layer (models, ORM) to Documentation
  • Has patch set
  • Version changed from 1.6 to master

Since this is an old ticket and there doesn't seem to be a straightforward way to fix it, I was inclined to close this in the documentation front.
PR: https://github.com/django/django/pull/2982
Thanks.

comment:4 Changed 13 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:5 Changed 13 months ago by Tim Graham <timograham@…>

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

In 668d432d0a25ef68e101f2c6492dc4d75da931bd:

Fixed #22491 -- documented how select_for_update() should be tested.

Thanks Andreas Pelme for the report.

comment:6 Changed 13 months ago by Tim Graham <timograham@…>

In 6c70b1d7df4bc6f666ee9fa55a305e7347862095:

[1.6.x] Fixed #22491 -- documented how select_for_update() should be tested.

Thanks Andreas Pelme for the report.

Backport of 668d432d0a from master

comment:7 Changed 13 months ago by Tim Graham <timograham@…>

In acbe3fac7c1210aacf2a931d9c5dde4f8fecd6c4:

[1.7.x] Fixed #22491 -- documented how select_for_update() should be tested.

Thanks Andreas Pelme for the report.

Backport of 668d432d0a from master

Note: See TracTickets for help on using tickets.
Back to Top