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

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]