On 3/21/22 17:58, Ilya Maximets wrote:
> On 3/21/22 15:45, David Marchand wrote:
>> On Mon, Mar 21, 2022 at 1:39 PM Ilya Maximets <[email protected]> wrote:
>>>
>>> OVS_WAIT_UNTIL() macro has only 2 arguments and doesn't check
>>> the output of the command, but bonding tests are trying to use
>>> it as if it was AT_CHECK macro. That makes checks in bonding
>>> tests mostly useless, since they are not actually checking
>>> anything except for command returning zero.
>>>
>>> Introducing a new macro OVS_WAIT_UNTIL_EQUAL that will actually
>>> perform the comparison with the desired output. Using it for
>>> the bonding tests and fixing all the caught incorrect expected
>>> outputs along the way.
>>
>> - Did you consider having the same semantic for OVS_WAIT_UNTIL() as
>> AT_CHECK(), rather than introduce a new helper?
>> I think it would be more unit tests developer friendly.
>
> Might make sense, I'll look into that.
> Changes in the macro will be heavier though.
I'm finding myself writing a fairly complex shell code with lots
of checks for existence of arguments and lots of different cases.
AT_CHECK is not that simple.
For now I'm inclined to keep the new macro and just add argument
validation in v2 for both new and old, so they will explicitly
fail on misuse. What do you think?
>
>>
>>
>> - There might also be some other broken checks, worth followup fix(es).
>> I caught one in tests/system-route.at:
>>
>> dnl Check that OVS catches route updates.
>> OVS_WAIT_UNTIL([ovs-appctl ovs/route/show | grep 'p1-route' | sort], [0],
>> [dnl
>> Cached: 10.0.0.17/24 dev p1-route SRC 10.0.0.17
>> Cached: 10.0.0.17/32 dev p1-route SRC 10.0.0.17 local
>> ])
>
> Good catch. I've seen this one, but lost track of it
> while preparing the actual patch.
>
>>
>>
>>>
>>> Fixes: b4e50218a0f8 ("bond: Add 'primary' interface concept for
>>> active-backup mode.")
>>> Signed-off-by: Ilya Maximets <[email protected]>
>>
>> Otherwise the patch lgtm.
>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
