URL:
https://github.com/SSSD/sssd/pull/947
Title: #947: tests: fix race conditions in integration tests
alexey-tikhonov commented:
"""
In regards of `test_files_provider.py`
There is common pattern like:
```
def check_group(exp_group, delay=3.0):
if delay > 0:
time.sleep(delay)
res, found_group = sssd_getgrnam_sync(exp_group["name"])
assert res == NssReturnCode.SUCCESS
assert found_group == exp_group
```
but `sssd_get..._sync()` calls `poll_canary()` which has its own "sleep"
(20*0.1 sec currently).
This seems to be original design (not introduced by this patch).
`poll_canary()` only awaits if result is `NOTFOUND` and has nice explanation of its
legitimacy in the comment.
But other sleeps are unconditional and its intention is not that clear. Some explanation
is given in the original commit message:
```
during testing especially on slow machines, it became apparent that
sometimes the inotify message arrives later than the test would check
for the changed entries. Therefore, the check would query the NSS
responder even before the sss-files domain was invalidated.
```
So it seems intention is to cover case when files content was *modified* (not
added/deleted). Like here:
```
modgroup = dict(GROUP1)
modgroup['name'] = 'group1_mod'
add_group_with_canary.groupmod(old_name=GROUP1["name"], **modgroup)
check_group(modgroup)
```
But IMO this hides a bug in a files provider: sssd *does* return stale data in this case.
And I am not sure glossing over this bug in the test is a good idea.
Some relevant to this problem thoughts can be found here:
https://bugzilla.redhat.com/show_bug.cgi?id=1693379#c6 (what we hit here IMO is more or
less the same race as mentioned there)
Given above, I would probably opt to:
- increase threshold in `poll_canary()`;
- get rid of all other sleeps
- and disable/mark "expected to fail" tests that currently expose bug in
files_provider & open ticket for files_provder.
OTOH I still see some value in those tests even with sleeps. It just looks bad...
"""
See the full comment at
https://github.com/SSSD/sssd/pull/947#issuecomment-559190489