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
BTW:
I can review patches next week due to higher priority task.