[Django] #32831: Flaky test in CI: cache.tests.BaseMemcachedTests.test_touch
Django
------------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
The following tests occasionally fail in CI:
* `cache.tests.PyLibMCCacheTests.test_touch`
* `cache.tests.PyMemcacheCacheTests.test_touch`
{{{
Traceback (most recent call last):
File "/home/jenkins/workspace/pull-requests-
bionic/database/sqlite3/label/bionic-
pr/python/python3.8/tests/cache/tests.py", line 481, in test_touch
self.assertIs(cache.touch('expire1'), True)
AssertionError: False is not True
}}}
Both test classes inherit from `BaseCacheTests`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32831>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Chris Jerdonek:
Old description:
> The following tests occasionally fail in CI:
>
> * `cache.tests.PyLibMCCacheTests.test_touch`
> * `cache.tests.PyMemcacheCacheTests.test_touch`
>
> {{{
> Traceback (most recent call last):
> File "/home/jenkins/workspace/pull-requests-
> bionic/database/sqlite3/label/bionic-
> pr/python/python3.8/tests/cache/tests.py", line 481, in test_touch
> self.assertIs(cache.touch('expire1'), True)
> AssertionError: False is not True
> }}}
>
> Both test classes inherit from `BaseCacheTests`.
New description:
The following tests occasionally fail in CI:
* `cache.tests.PyLibMCCacheTests.test_touch`
* `cache.tests.PyMemcacheCacheTests.test_touch`
{{{
Traceback (most recent call last):
File "/home/jenkins/workspace/pull-requests-
bionic/database/sqlite3/label/bionic-
pr/python/python3.8/tests/cache/tests.py", line 481, in test_touch
self.assertIs(cache.touch('expire1'), True)
AssertionError: False is not True
}}}
The line that fails is here:
https://github.com/django/django/blob/ed3af3ff4b3cfb72de598f1b39a1028ba3826efb/tests/cache/tests.py#L481
Both test classes inherit from `BaseCacheTests`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:1>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
Yes these tests are flaky (and probably 2-3 more cache tests). We tried to
fix them in the past but with no success (see
[https://github.com/django/django/pull/13496 PR]).
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:2>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Chris Jerdonek:
Old description:
> The following tests occasionally fail in CI:
>
> * `cache.tests.PyLibMCCacheTests.test_touch`
> * `cache.tests.PyMemcacheCacheTests.test_touch`
>
> {{{
> Traceback (most recent call last):
> File "/home/jenkins/workspace/pull-requests-
> bionic/database/sqlite3/label/bionic-
> pr/python/python3.8/tests/cache/tests.py", line 481, in test_touch
> self.assertIs(cache.touch('expire1'), True)
> AssertionError: False is not True
> }}}
>
> The line that fails is here:
> https://github.com/django/django/blob/ed3af3ff4b3cfb72de598f1b39a1028ba3826efb/tests/cache/tests.py#L481
>
> Both test classes inherit from `BaseCacheTests`.
New description:
The following tests occasionally fail in CI:
* `cache.tests.PyLibMCCacheTests.test_touch`
* `cache.tests.PyMemcacheCacheTests.test_touch`
{{{
Traceback (most recent call last):
File "/home/jenkins/workspace/pull-requests-
bionic/database/sqlite3/label/bionic-
pr/python/python3.8/tests/cache/tests.py", line 481, in test_touch
self.assertIs(cache.touch('expire1'), True)
AssertionError: False is not True
}}}
The line that fails is here:
https://github.com/django/django/blob/ed3af3ff4b3cfb72de598f1b39a1028ba3826efb/tests/cache/tests.py#L481
Both test classes inherit from `BaseMemcachedTests`, which inherits from
`BaseCacheTests`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:3>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Could this be due to different concrete subclasses of `BaseMemcachedTests`
stepping on each other during a parallel run?
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:4>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Another cache-related flaky test is:
`cache.tests.CacheMiddlewareTest.test_cache_page_timeout`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:5>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
A possible pattern for fixing issues like this would be to keep retrying
the assertion in a `while` loop, with a small sleep in between each
attempt, and a relatively large cumulative amount of time before giving
up. That would make it as pass as quickly as possible, without waiting
longer than necessary.
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:6>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Something else that occurred to me is that it could only help to start
each test by asserting that each key being used isn't set, and to end each
test by clearing the key in a finally block. The former would help to show
if a test isolation issue is at fault.
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:8>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Another flaky test failure: `test_forever_timeout()` at this line--
https://github.com/django/django/blob/1697098795707bd39501d9ceddd3d9be93f8d549/tests/cache/tests.py#L606
(again with `pull-requests-bionic/database=spatialite,label=bionic-
pr,python=python3.8`).
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:10>
Django
Reporter: Chris Jerdonek | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: Chris Jerdonek => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia Bidart):
I started some debugging efforts [https://forum.djangoproject.com/t/flaky-
cache-tests/26990/4 in this forum post] and landed a fix for `test_touch`
when using [https://github.com/django/django/pull/17614 the filebased
cache backend].
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia Bidart):
{{{
> git init /home/jenkins/workspace/main-
random/database/spatialite/label/focal/python/python3.12 # timeout=10
...
Using shuffle seed: 5744808359 (generated)
...
======================================================================
FAIL [0.004s]: test_lru_get (cache.tests.LocMemCacheTests.test_lru_get)
----------------------------------------------------------------------
random/database/spatialite/label/focal/python/python3.12/django/test/utils.py",
line 446, in inner
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/jenkins/workspace/main-
random/database/spatialite/label/focal/python/python3.12/tests/cache/tests.py",
line 1406, in test_lru_get
self.assertEqual(cache.get(key), key)
AssertionError: None != 0
----------------------------------------------------------------------
Ran 17769 tests in 1613.662s
FAILED (failures=1, skipped=1160, expected failures=5)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:13>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia Bidart):
{{{
...
Using shuffle seed: 5526860306 (generated)
...
======================================================================
ERROR [0.003s]: test_lru_incr (cache.tests.LocMemCacheTests.test_lru_incr)
----------------------------------------------------------------------
random/database/spatialite/label/focal/python/python3.12/django/test/utils.py",
line 446, in inner
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/jenkins/workspace/main-
random/database/spatialite/label/focal/python/python3.12/tests/cache/tests.py",
self.assertEqual(cache.incr(key), key + 1)
^^^^^^^^^^^^^^^
File "/home/jenkins/workspace/main-
random/database/spatialite/label/focal/python/python3.12/django/core/cache/backends/locmem.py",
line 70, in incr
raise ValueError("Key '%s' not found" % key)
ValueError: Key ':1:0' not found
----------------------------------------------------------------------
Ran 17776 tests in 1566.550s
FAILED (errors=1, skipped=1161, expected failures=5)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Component: Core (Cache system) | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I get a sense that a lot of these failures are due to tests using their
same global store at the same time as Chris pointed out. A clear sign of
that is `cache.clear` is being called at `setUp` time.
A solution here could be to run these tests cases in they own separate key
space (e.g. with a cache key prefix or with two distinct memcached
instances) or to serialize them to ensure that no tests share the same
resource (e.g. using a `multiprocessing.lock` acquired in `setUp` and
released in `tearDown`).
There might be more too it though as I'm unsure what the djangci setup
looks like. Is there one memcached / Redis instance per job or are they
shared amongst them?
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:15>
Django
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Wassef Ben Ahmed):
* owner: (none) => Wassef Ben Ahmed
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Wassef Ben Ahmed):
to my understanding,
so far we don't know which tests exactly are flaky.
mainly the cases mentioned above, and anything that uses a shared medium.
as the @Simon Charette suggested this could be done by figuring out where
to set locks, which he also pointed out that it might get tricky depending
on how djangoci works (I have no clue)
we might need to handle lock depending on the platform running the test or
use something like "portalocker" which covers cross-platform locks but
forces the adoption of a new dependency?
expanding on a previous answer by @Chris Jerdonek on setting assertion
retries, why not set it at a unit level instead of individual assertion?
how you'd typically do with some pytest extensions... And we'll be
assured the whole test passed without having to reason line-by-line on
what went wrong?
implementation goes something like:
{{{
from functools import wraps
def retry(retries=3, delay=1):
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
attempts = 0
while attempts < retries:
try:
return func(*args, **kwargs)
except AssertionError as e:
attempts += 1
if attempts >= retries:
raise
time.sleep(delay)
return wrapper
return decorator
}}}
only catching/retrying on "AssertionError" so other exceptions are not
permitted.
which I'm still not sure about? (maybe a shared medium causes you to pluck
a None or something...)
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Wassef Ben Ahmed):
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Natalia Bidart):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Wassef Ben Ahmed):
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
| checkin
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"957c54d945fedb58febff05e4ce82377cc43f9f4" 957c54d9]:
{{{#!CommitTicketReference repository=""
revision="957c54d945fedb58febff05e4ce82377cc43f9f4"
Fixed #32831 -– Allowed cache tests to be retried via a new "retry"
decorator.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Wassef
Type: | Ben Ahmed
Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia <124304+nessita@…>):
In [changeset:"33cd593420c78a0196d03edf48de9d281d7a3a37" 33cd593]:
{{{#!CommitTicketReference repository=""
revision="33cd593420c78a0196d03edf48de9d281d7a3a37"
[5.1.x] Fixed #32831 -– Allowed cache tests to be retried via a new
"retry" decorator.
Backport of 957c54d945fedb58febff05e4ce82377cc43f9f4 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32831#comment:23>