Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the fixes.
Please, consider my comments below.

On 10.11.23, Sergey Bronnikov wrote:
> Hi, Igor
> 
> On 11/1/23 11:28, Igor Munkin via Tarantool-patches wrote:
> > Sergey,
> >
> > On 01.11.23, Sergey Kaplun via Tarantool-patches wrote:
> >> Hi, Sergey!
> > <snipped>
> >
> >> If some test fails we got the following output:
> >>
> >> | TAP version 13
> >> | 1..1
> >> | not ok 1 - lua_concat_testcase
> >> |   ---
> >> |   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
> >> |   failed_assertion:     assert_str_equal
> >> |   got: 1652880104
> >> |   expected: -1138225337
> >> |   ...
> >> | # Failed 1 tests out of 1
> >>
> >> Failed assertion field is incorrect (see the comment above).
> >> But the most important is "got" and "expected" fields, that returns the
> >> addresses of strings, which isn't very meaningful.
> >>
> >> I suggest dumping the strings instead if they are not long enough (less
> >> than 128 symbols, I suppose). The maxdump string length should be
> >> a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
> >> inside the <test.h> header. So the user can redefine it before the
> >> `assert_str_{not_}equal()` and use a custom value.
> >>
> >> If the string is too long, we should dump the offset of the mismatched
> >> symbol. Or maybe it's better to always dump it.
> >>
> >> Thoughts?
> > What if the different part starts after 128 symbols? I believe the most
> > valuable part is the one where the difference starts, so you have to
> > dump the beginning (for convenience), the difference and some numeric
> > parameters (length, offset where strings differ, etc).
> >
> > Furthermore, I suggest implementing <*_str_*> helpers for nul-terminated
> > strings and <*_mem_*> helpers for length limited memory blobs.
> I would postpone implementation of  <*_mem_*> helpers until we will need 
> them in tests.

I'm totally OK with it.

> >
> >> Side note:
> >>
> >> Also, this comparing "by eye" isn't very convenient if values aren't
> >> aligned, so maybe it is better to use spaces instead of tabs to align
> >> values? This may be added within the separate patch series later.
> > I believe, this is quite minor thing (at least for now).
> >
> > <snipped>
> >
> >> -- 
> >> Best regards,
> >> Sergey Kaplun

I'll proceed with the review here with the diff below:

> index 462534be..8fad6407 100644
> --- a/test/tarantool-c-tests/README.md
> +++ b/test/tarantool-c-tests/README.md
> @@ -35,6 +35,8 @@ glibc `assert()`:
>    equal to the `b`.
>  * `assert_double{_not}_equal(a, b)` -- check that two doubles are {not}
>    **exactly** equal.
> +* `assert_str{_not}_equal(a, b)` -- check that `char *` variable `a` is {not}
> +  equal to the `b`.
>  
>  ## Directives
>  
> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> index 8b14c705..2f2d9ea2 100644
> --- a/test/tarantool-c-tests/test.h
> +++ b/test/tarantool-c-tests/test.h
> @@ -13,8 +13,6 @@
>   * * Helpers assert macros:
>   *   - assert_uint_equal if needed
>   *   - assert_uint_not_equal if needed
> - *   - assert_str_equal if needed
> - *   - assert_str_not_equal if needed
>   *   - assert_memory_equal if needed
>   *   - assert_memory_not_equal if needed
>   * * Pragmas.
> @@ -214,4 +212,18 @@ static inline int todo(const char *reason)
>          );                                                              \
>  } while (0)
>  
> +#define assert_str_equal(got, expected, n) do {                         \
> +        assert_general(strncmp(got, expected, n) == 0,                  \

Maybe it is better to just use `strcmp(got, expected)` instead?
We don't really care about the number of characters to check.

Or we can use `strncmp(got, expected, strlen(expected))`, as the most
common case.

This removes an additional argument, and the description in the
<README.md> and declaration here become the same.

> +                       ASSERT_EQUAL_FMT(str, "%s"),                     \
> +                       __FILE__, __LINE__, (got), (expected)            \
> +        );                                                              \
> +} while (0)
> +
> +#define assert_str_not_equal(got, unexpected, n) do {                   \
> +        assert_general(strncmp(got, expected, n) != 0,                  \

Ditto.

> +                       ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
> +                       __FILE__, __LINE__, (got), (unexpected)          \
> +        );                                                              \
> +} while (0)
> +
>  #endif /* TARANTOOL_LUAJIT_TEST_H */

-- 
Best regards,
Sergey Kaplun