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