Flush cache after the current transaction is committed (#296) #300
Conversation
dtao
force-pushed
the
invalidate-cache-on-commit
branch
from
bc36a48
to
b2ea258
Compare
| @@ -3,14 +3,14 @@ | |||
| from decimal import Decimal | |||
|
|
|||
| from django.contrib.auth.models import AnonymousUser | |||
| from django.test import TestCase, RequestFactory | |||
| from django.test import TransactionTestCase, RequestFactory | |||
There was a problem hiding this comment.
Django's TestCase class automatically wraps each test method in a transaction and rolls back the transaction after the method returns. Since updating models now doesn't flush the cache until after the transaction is committed, this breaks the logic in most of these tests. Inheriting from TransactionTestCase addresses the problem by removing Django's magical auto-rollback behavior, the trade-off being that the tests are a tad slower.
|
I suppose you could technically merge this before merging #299; there would just effectively be no test coverage since the test added in this PR is skipped when running tests with sqlite. |
waffle/tests/test_waffle.py
Outdated
| 'This test uses threads, which the sqlite3 DB engine ' | ||
| 'does not support.') | ||
| def test_update_switch_in_transaction(self): | ||
| """Wait to invalidate the cache until after the current transaction.""" |
There was a problem hiding this comment.
I appreciate all the comments here, this is definitely not an obvious issue.
Would you mind adding a slightly longer description to the doc block (as a second paragraph) and include a link to #296?
Should we have similar tests for Flag and Sample? I know the behavior exists in BaseModel but that's an implementation detail, not a public API. If we needed to refactor it, especially in support of extensible models, it could change or break.
There was a problem hiding this comment.
For sure, I can add another comment explicitly referencing #296.
I'm also happy to add equivalent tests for Flag and Sample.
dtao
force-pushed
the
invalidate-cache-on-commit
branch
from
b2ea258
to
52d3b4a
Compare
|
@jsocol Per your suggestions I've added an explanatory comment to the somewhat complex test method to cover this case and also updated the test code to cover flags, switches, and samples. I squashed this work into one commit because I think that's what you prefer, though I acknowledge that can make it hard to tell what changed since you last reviewed the code. If I've misunderstood, and for future reference I should not squash commits in pull requests that are under review, let me know. |
|
Is there any interest in releasing a new version with this fix? We are just depending on a fork at the moment. It's always nice to return to the fold when we can :) |
This builds off the work in pull request #299 and fixes the issue raised in #296.
Note: review #299 before reviewing this.