[ovs-dev] [PATCH v3 1/3] ovsdb-idl.at: Make test outputs more predictable.

[Dumitru Ceara]

On 3/18/21 6:30 PM, Ilya Maximets wrote:
> On 3/12/21 1:07 PM, Dumitru Ceara wrote:
>> IDL tests need predictable output from test-ovsdb.
>>
>> This used to be done by first sorting the output of test-ovsdb and then
>> applying uuidfilt to predictably translate UUIDs.  This was not
>> reliable enough in case test-ovsdb processes two or more insert/delete
>> operations in the same iteration because the order of lines in the
>> output depends on the automatically generated UUID values.
>>
>> To fix this we change the way test-ovsdb and test-ovsdb.py generate
>> outputs and prepend the table name and tracking information before
>> printing the contents of a row.
>>
>> All existing ovsdb-idl.at and ovsdb-cluster.at tests are updated to
>> expect the new output format.
>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>> Note: the old approach was enough for outputs of the existing tests but
>> the next patch in this series adds a new test that requires this
>> change.
>>
>> v3:
>> - Changed expected output of ovsdb-cluster.at to reflect the new
>>   formatting in test-ovsdb output.
>> - Fixed typo in test-ovsdb.py.
>> v2:
>> - Reworked the patch and changed test-ovsdb.c and test-ovsdb.py to
>>   generate output that can be sorted predictably.
>> - Rephrased commit message.
>> ---
>>  lib/ovsdb-idl.c        |    3 
>>  lib/ovsdb-idl.h        |    2 
>>  tests/ovsdb-cluster.at |    2 
>>  tests/ovsdb-idl.at     |  463 +++++++++++++++++++++++-------------------------
>>  tests/test-ovsdb.c     |  186 +++++++++++--------
>>  tests/test-ovsdb.py    |   87 +++++----
>>  6 files changed, 390 insertions(+), 353 deletions(-)
>>
>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
>> index 2c8a0c9..9e1e787 100644
>> --- a/lib/ovsdb-idl.c
>> +++ b/lib/ovsdb-idl.c
>> @@ -182,7 +182,6 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *,
>>  static struct ovsdb_idl_table *
>>  ovsdb_idl_table_from_class(const struct ovsdb_idl *,
>>                             const struct ovsdb_idl_table_class *);
>> -static bool ovsdb_idl_track_is_set(struct ovsdb_idl_table *table);
>>  static void ovsdb_idl_track_clear__(struct ovsdb_idl *, bool flush_all);
>>  
>>  static void ovsdb_idl_destroy_indexes(struct ovsdb_idl_table *);
>> @@ -1140,7 +1139,7 @@ ovsdb_idl_track_add_all(struct ovsdb_idl *idl)
>>  }
>>  
>>  /* Returns true if 'table' has any tracked column. */
>> -static bool
>> +bool
>>  ovsdb_idl_track_is_set(struct ovsdb_idl_table *table)
>>  {
>>      size_t i;
>> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
>> index 05bb48d..d934832 100644
>> --- a/lib/ovsdb-idl.h
>> +++ b/lib/ovsdb-idl.h
> 
> <snip>
> 
>> @@ -1538,44 +1527,44 @@ OVSDB_CHECK_IDL_NOTIFY([simple idl verify notify],
>>         "where": [["i", "==", 0]]}]' \
>>      'reconnect']],
>>    [[000: empty
>> -000: event:create, row={uuid=<0>}, updates=None
>> -000: event:create, row={uuid=<1>}, updates=None
>> -001: {"error":null,"result":[{"uuid":["uuid","<2>"]},{"uuid":["uuid","<3>"]}]}
>> -002: event:create, row={i=0 r=0 b=false s= u=<4> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>}, updates=None
>> -002: event:create, row={i=1 r=2 b=true s=mystring u=<5> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<6> <7>] uuid=<2>}, updates=None
>> -002: i=0 r=0 b=false s= u=<4> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
>> -002: i=1 r=2 b=true s=mystring u=<5> ia=[1 2 3] ra=[-0.5] ba=[true] sa=[abc def] ua=[<6> <7>] uuid=<2>
>> +000: event:create, row={}, updates=None
>> +000: event:create, row={}, updates=None
> 
> This doesn't look right to remove uuid from here.

You're right, sorry about this, it wasn't intended like this.

> Suggesting following change to bring it back
> (this patch also needs a slight rebase):
> 
> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
> index 9d3228f23..bc2be6acf 100644
> --- a/tests/test-ovsdb.py
> +++ b/tests/test-ovsdb.py
> @@ -640,8 +653,8 @@ def do_idl(schema_file, remote, *commands):
>      def mock_notify(event, row, updates=None):
>          output = "%03d: " % step
>          output += "event:" + str(event) + ", row={"
> -        output += get_simple_table_printable_row(row,
> -                                                 'l2', 'l1') + "}, updates="
> +        output += get_simple_table_printable_row(row, 'l2', 'l1') + "}, "
> +        output += get_simple_printable_row_string(row, ["uuid"]) + ", updates="
>          if updates is None:
>              output += "None"
>          else:
> ---

Sure, I'll add this to v4.

> 
> Otherwise, it looks fine.
> 
> Best regards, Ilya Maximets.
> 

Thanks,
Dumitru