[SSSD] [PATCH] tests: be_ptask

Michal Židek mzidek at redhat.com
Thu Nov 20 17:46:30 UTC 2014


On 11/20/2014 06:04 PM, Lukas Slebodnik wrote:
> On (20/11/14 17:41), Pavel Březina wrote:
>> All above issues where addressed.
>>
>>>> Very nice unit test.
>>
>> Thank you.
>>
>>>> It would be good to add few more tests with corner cases.
>>>> There aren't test for failed functions (_send, _recv)
>>>> (Attached is diff with test for error occured in be_ptask_sync_t.)
>>
>> Thank you for the diff, I have rewritten it slightly. We don't really care
>> that the tevent_req error was set to the return code of yours - that is
>> hidden to the caller. What we want to check is that the task will be
>> correctly rescheduled according to documentation.
>>
>> This test also revealed a huge bug that we are quite lucky we haven't been
>> punished for it so far. So cudos. (current patch #4).
>>
>> I added new tests for corner cases (_recv returns error is already there as
>> *reschedule_error):
>> - test_be_ptask_reschedule_null - _send returns NULL
>> - test_be_ptask_reschedule_timeout - req timed out
>> - test_be_ptask_sync_reschedule_ok - sync task returns EOK
>> - test_be_ptask_sync_reschedule_error - sync task returns error
>>
>>>> There are 6 "time_t" arguments be_ptask_create.
>>>> perriod and first_delay are tested.
>>>> enabled_delay,
>>
>> enabled_delay is tested as well.
>>
>>
>> random_offset, timeout, max_backoff are tested just with zero.
>>>> It would be good to add tests at least for timeout, max_backoff.
>>>> The most lines without code coverage are due to this two variables.
>>
>> Digging now into the implementation of the backoff feature, I believe it is
>> wrong. It seems to do what it is supposed to, however it overrides even
>> first_delay and enabled delay. We should fix that... or was it an intended
>> behaviour?
>>
> Michal,
> that's question for you because you implemented
> features random_offset, max_backoff.
>
> LS

This was intentional. I though it is better to calculate the new
interval based on the "period", not the enabled_delay or first_delay.
I should have write this to comments probably. The reason was,
that otherwise all next delays would be derived from enabled_delay or
first_delay, which I though was not intuitive.

But now I think I should have done this the other way around,
ignoring the "period" value and use only enabled/first_delay.
Because the task is not really periodic anyway. I'll send a
patch to fix this and clarify in comments what parameters are
used and what are not if the backoff is enabled.

Thanks for noticing.

>
> BTW:
> I can review patches next week due to higher priority task.




More information about the sssd-devel mailing list