Opened 3 years ago

Last modified 3 years ago

#33161 closed New feature

Do not ignore transaction durability errors within TestCase — at Version 11

Reported by: Krzysztof Jagiełło Owned by: Krzysztof Jagiełło
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: David Seddon, Adam Johnson, Ian Foote, Hannes Ljungberg 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 (last modified by Krzysztof Jagiełło)

Currently there is a discrepancy in how durable atomic blocks are handled in TransactionTestCase vs TestCase. Using the former, nested durable atomic blocks will, as expected, result in a RuntimeError. Using the latter however, the error will go unnoticed as the durability check is turned off.

I have faced some issues with this behaviour in a codebase where we heavily utilize TestCase and where we recently started to introduce durable atomic blocks – the durability errors do not surface until the code hits staging/production. The solution could be to switch over to TransactionTestCase for the test classes that hit code paths with durable atomic blocks, but having to identify which tests could be affected by this issue is a bit inconvenient. And then there is the performance penalty of using TransactionTestCase.

So, to the issue at hand. The durability check is disabled for TestCase because otherwise durable atomic blocks would fail immediately as TestCase wraps its tests in transactions. We could however add a marker to the transactions created by TestCase, keep a stack of active transactions and make the durability check take the stack of transactions with their respective markers into account. This way we could easily detect when a durable atomic block is directly within a transaction created by TestCase and skip the durability check only for this specific scenario.

To better illustrate what I am proposing here, I have prepared a PoC patch. Let me know what you think!

Original PoC patch: https://github.com/django/django/pull/14919
Current patch: https://github.com/django/django/pull/14922

Change History (11)

comment:1 by Krzysztof Jagiełło, 3 years ago

Description: modified (diff)

comment:2 by Krzysztof Jagiełło, 3 years ago

Owner: changed from nobody to Krzysztof Jagiełło
Status: newassigned

comment:3 by Krzysztof Jagiełło, 3 years ago

Version: 3.2dev

comment:4 by Mariusz Felisiak, 3 years ago

Cc: David Seddon Adam Johnson Ian Foote added

I don't think it's worth additional complexity and potential slowdown of TestCase for all users. I will wait for a second opinion before making a decision.

comment:5 by Krzysztof Jagiełło, 3 years ago

Could you elaborate on what would cause the slowdown? I have noticed the mention about it in the docs, but couldn't really figure out why this would incur any significant performance penalty.

Last edited 3 years ago by Krzysztof Jagiełło (previous) (diff)

comment:6 by Hannes Ljungberg, 3 years ago

Cc: Hannes Ljungberg added

comment:7 by Adam Johnson, 3 years ago

I think it's reasonable for tests to still detect such problematic durable atomic blocks, without requiring TransactionTestCase.

I've reviewed the initial PR and I came up with an idea for a simpler implementation: https://github.com/django/django/pull/14922

My implementation removes ensure_durability and adds another boolean flag on BaseDatabaseWrapper. This leaves us with about the same level of complexity as before the PR, so hopefully that alleviates any concerns.

(I also don't believe there's any potential for notable slowdown from the initial implementation - it only adds one stack.)

comment:8 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

(I also don't believe there's any potential for notable slowdown from the initial implementation - it only adds one stack.)

Persuaded.

comment:9 by Krzysztof Jagiełło, 3 years ago

Description: modified (diff)

comment:10 by Krzysztof Jagiełło, 3 years ago

Thanks Adam! Your approach seems more straightforward to me than the initial implementation. Keeping around the stack of atomic blocks just to handle this is specific scenario was a bit unnecessary.

comment:11 by Krzysztof Jagiełło, 3 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top