[v2,2/3] Documentation: KUnit: reword description of assertions

The existing wording implies that kunit_kmalloc_array() is "the method
under test". We're actually testing the sort() function in that example.
This is because the example was changed in commit 953574390634
("Documentation: KUnit: Rework writing page to focus on writing tests"),
but the wording was not.

Also add a `note` telling people they can use the KUNIT_ASSERT_EQ()
macros from any function. Some users might be coming from a framework
like gUnit where that'll compile but silently do the wrong thing.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 Documentation/dev-tools/kunit/usage.rst | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

On Wed, Nov 9, 2022 at 6:06 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> The existing wording implies that kunit_kmalloc_array() is "the method
> under test". We're actually testing the sort() function in that example.
> This is because the example was changed in commit 953574390634
> ("Documentation: KUnit: Rework writing page to focus on writing tests"),
> but the wording was not.
>
> Also add a `note` telling people they can use the KUNIT_ASSERT_EQ()
> macros from any function. Some users might be coming from a framework
> like gUnit where that'll compile but silently do the wrong thing.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Thank you, Daniel. This looks fine to me except for a small typo in
this line "to abort
the test if we there's an allocation error". Also, I have reworded
that paragraph a bit
as below. Please feel free to ignore, if you do not agree:

In this example, to test the ``sort()`` function, we must be able to
allocate an array.
If there is an allocation error, the test is terminated using the function
``KUNIT ASSERT NOT ERR OR NULL()``.

Reviewed-by: Sadiya Kazi <sadiyakazi@google.com>

Best Regards,
Sadiya



>  Documentation/dev-tools/kunit/usage.rst | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index b0a6c3bc0eeb..8060114e3aa6 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -112,11 +112,14 @@ terminates the test case if the condition is not satisfied. For example:
>                         KUNIT_EXPECT_LE(test, a[i], a[i + 1]);
>         }
>
> -In this example, the method under test should return pointer to a value. If the
> -pointer returns null or an errno, we want to stop the test since the following
> -expectation could crash the test case. `ASSERT_NOT_ERR_OR_NULL(...)` allows us
> -to bail out of the test case if the appropriate conditions are not satisfied to
> -complete the test.
> +In this example, we need to be able to allocate an array to test the ``sort()``
> +function. So we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()`` to abort the test if
> +we there's an allocation error.
> +
> +.. note::
> +   In other test frameworks, ``ASSERT`` macros are often implemented by calling
> +   ``return`` so they only work from the test function. In KUnit, we stop the
> +   current kthread on failure, so you can call them from anywhere.
>
>  Customizing error messages
>  --------------------------
> --
> 2.38.1.431.g37b22c650d-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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221109003618.3784591-2-dlatypov%40google.com.

On Wed, Nov 9, 2022 at 9:07 PM Sadiya Kazi <sadiyakazi@google.com> wrote:
>
> On Wed, Nov 9, 2022 at 6:06 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > The existing wording implies that kunit_kmalloc_array() is "the method
> > under test". We're actually testing the sort() function in that example.
> > This is because the example was changed in commit 953574390634
> > ("Documentation: KUnit: Rework writing page to focus on writing tests"),
> > but the wording was not.
> >
> > Also add a `note` telling people they can use the KUNIT_ASSERT_EQ()
> > macros from any function. Some users might be coming from a framework
> > like gUnit where that'll compile but silently do the wrong thing.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Thank you, Daniel. This looks fine to me except for a small typo in
> this line "to abort
> the test if we there's an allocation error". Also, I have reworded
> that paragraph a bit
> as below. Please feel free to ignore, if you do not agree:
>
> In this example, to test the ``sort()`` function, we must be able to
> allocate an array.
> If there is an allocation error, the test is terminated using the function
> ``KUNIT ASSERT NOT ERR OR NULL()``.

Thanks for catching that.

Hmm, I slightly prefer the current structure since I like having the
<thing> being described near the start of the sentence as opposed to
the very end.
I'll wait a bit before sending a v3 to give time for anyone else to
chime in, if they want.

Snipping the email to the block in question:

> > +In this example, we need to be able to allocate an array to test the ``sort()``
> > +function. So we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()`` to abort the test if
> > +we there's an allocation error.

On Fri, Nov 11, 2022 at 12:04 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Wed, Nov 9, 2022 at 9:07 PM Sadiya Kazi <sadiyakazi@google.com> wrote:
> >
> > On Wed, Nov 9, 2022 at 6:06 AM 'Daniel Latypov' via KUnit Development
> > <kunit-dev@googlegroups.com> wrote:
> > >
> > > The existing wording implies that kunit_kmalloc_array() is "the method
> > > under test". We're actually testing the sort() function in that example.
> > > This is because the example was changed in commit 953574390634
> > > ("Documentation: KUnit: Rework writing page to focus on writing tests"),
> > > but the wording was not.
> > >
> > > Also add a `note` telling people they can use the KUNIT_ASSERT_EQ()
> > > macros from any function. Some users might be coming from a framework
> > > like gUnit where that'll compile but silently do the wrong thing.
> > >
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > Thank you, Daniel. This looks fine to me except for a small typo in
> > this line "to abort
> > the test if we there's an allocation error". Also, I have reworded
> > that paragraph a bit
> > as below. Please feel free to ignore, if you do not agree:
> >
> > In this example, to test the ``sort()`` function, we must be able to
> > allocate an array.
> > If there is an allocation error, the test is terminated using the function
> > ``KUNIT ASSERT NOT ERR OR NULL()``.
>
> Thanks for catching that.
>
> Hmm, I slightly prefer the current structure since I like having the
> <thing> being described near the start of the sentence as opposed to
> the very end.
> I'll wait a bit before sending a v3 to give time for anyone else to
> chime in, if they want.
>
> Snipping the email to the block in question:
>
> > > +In this example, we need to be able to allocate an array to test the ``sort()``
> > > +function. So we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()`` to abort the test if
> > > +we there's an allocation error.

+1 for the patch from me (modulo the "we" typo Sadiya mentioned).

I otherwise also prefer Daniel's original here (though I'd possibly
merge it into one sentence, personally).
Maybe:
"In this example, as we need to be able to allocate an array in order
to test the sort function, we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()``
to abort the test if there's an allocation error."
or
"In this example, we need to allocate an array to test the sort
function. We therefore use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()``, which
will automatically abort the test if there's an allocation error."

But any of the above wordings are fine for me.

The note about ASSERT() working in any function is useful, though
there are definitely some "gotcha"s caused by killing the kthread
we'll need to resolve. (If there are any dangling references to things
on the stack, for example.) Still, not an issue for this bit of
documentation.

Reviewed-by: David Gow <davidgow@google.com>

(Once the "we" typo is fixed.)

Cheers,
-- David

On Mon, Nov 14, 2022 at 11:45 PM David Gow <davidgow@google.com> wrote:

<snip>

> +1 for the patch from me (modulo the "we" typo Sadiya mentioned).
>
> I otherwise also prefer Daniel's original here (though I'd possibly
> merge it into one sentence, personally).
> Maybe:
> "In this example, as we need to be able to allocate an array in order
> to test the sort function, we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()``
> to abort the test if there's an allocation error."
> or
> "In this example, we need to allocate an array to test the sort
> function. We therefore use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()``, which
> will automatically abort the test if there's an allocation error."
>
> But any of the above wordings are fine for me.
>
> The note about ASSERT() working in any function is useful, though
> there are definitely some "gotcha"s caused by killing the kthread
> we'll need to resolve. (If there are any dangling references to things
> on the stack, for example.) Still, not an issue for this bit of
> documentation.
>
> Reviewed-by: David Gow <davidgow@google.com>
>
> (Once the "we" typo is fixed.)

v3 is here, PTAL
https://lore.kernel.org/all/20221111182906.1377191-2-dlatypov@google.com/

Copying the relevant section here:
+In this example, we need to be able to allocate an array to test the ``sort()``
+function. So we use ``KUNIT_ASSERT_NOT_ERR_OR_NULL()`` to abort the test if
+there's an allocation error.
+
+.. note::
+   In other test frameworks, ``ASSERT`` macros are often implemented by calling
+   ``return`` so they only work from the test function. In KUnit, we stop the
+   current kthread on failure, so you can call them from anywhere.

Daniel