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.
>
>
> - 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
