Message-ID: <CAGS_qxqnxzm1+45Mbufsz51mkz0_2PUt0eQiviwdGEV0+=ywKg@mail.gmail.com>
Date:   Mon, 10 Jan 2022 14:35:06 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     davidgow@...gle.com, linux-kernel@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        skhan@...uxfoundation.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH 4/6] kunit: factor out kunit_base_assert_format() call
 into kunit_fail()

On Mon, Jan 10, 2022 at 2:32 PM 'Brendan Higgins' via KUnit
Development <kunit-dev@...glegroups.com> wrote:
>
> On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@...gle.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@...gle.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.

The child patch renames it to kunit_assert_prologue().
I can rename it in this change if you prefer.

I had just initially left it with the same name to keep this diff a
bit smaller and more focused.
But now you point it out, I think it would be cleaner to rename it here.

>
> >         assert->format(assert, stream);
> >
> >         kunit_print_string_stream(test, stream);
> > --
> > 2.34.1.575.g55b058a8bb-goog
> >
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g47r8aQBWPtt6ffHokqqN2sMi10p1Q5QA3xGVLTVDQh98Q%40mail.gmail.com.

Powered by blists - more mailing lists