Re: [PATCH 4/6] kunit: factor out kunit_base_assert_format() call into kunit_fail()

[Brendan Higgins <brendanhiggins@google.com>]

On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.

nit: drop the "this".

> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
>  lib/kunit/assert.c | 6 ------
>  lib/kunit/test.c   | 1 +
>  2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index b972bda61c0c..4d9a1295efc7 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
>  void kunit_fail_assert_format(const struct kunit_assert *assert,
>                               struct string_stream *stream)
>  {
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream, "%pV", &assert->message);
>  }
>  EXPORT_SYMBOL_GPL(kunit_fail_assert_format);
> @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
>
>         unary_assert = container_of(assert, struct kunit_unary_assert, assert);
>
> -       kunit_base_assert_format(assert, stream);
>         if (unary_assert->expected_true)
>                 string_stream_add(stream,
>                                   KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n",
> @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>         ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert,
>                                   assert);
>
> -       kunit_base_assert_format(assert, stream);
>         if (!ptr_assert->value) {
>                 string_stream_add(stream,
>                                   KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
> @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_ptr_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_str_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 5ad671745483..735c1b67d843 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>                 return;
>         }
>
> +       kunit_base_assert_format(assert, stream);

I think my thinking in having this function called by the other assert
functions was to take advantage of inheritance. I was treating
kunit_base_assert_format as the parent method that other methods were
inheriting from, so I wanted to have them inherit some of the common
behavior by calling the original function.

If you decide to make this change, I think it would be a good idea to
change the name of kunit_base_assert_format to not mislead to this
effect.

>         assert->format(assert, stream);
>
>         kunit_print_string_stream(test, stream);
> --
> 2.34.1.575.g55b058a8bb-goog
>