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.