Re: [PATCH v1 1/3] kunit: Add kunit_add_action() to defer a call until test exit

[David Gow]

On Wed, 26 Apr 2023 at 10:12, Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Fri, Apr 21, 2023 at 1:42 AM David Gow <davidgow@xxxxxxxxxx> wrote:
> >
> > Many uses of the KUnit resource system are intended to simply defer
> > calling a function until the test exits (be it due to success or
> > failure). The existing kunit_alloc_resource() function is often used for
> > this, but was awkward to use (requiring passing NULL init functions, etc),
> > and returned a resource without incrementing its reference count, which
> > -- while okay for this use-case -- could cause problems in others.
> >
> > Instead, introduce a simple kunit_add_action() API: a simple function
> > (returning nothing, accepting a single void* argument) can be scheduled
> > to be called when the test exits. Deferred actions are called in the
> > opposite order to that which they were registered.
> >
> > This mimics the devres API, devm_add_action(), and also provides
> > kunit_remove_action(), to cancel a deferred action, and
> > kunit_release_action() to trigger one early.
>
> Apologies for the delayed bikeshedding.
>
> I think mimicking the devres API is a better idea than kunit_defer()
> and friends.
> But I can't help but think this still isn't the best name.
> I personally would have no idea what `kunit_release_action()` does
> without looking it up.
>
> I feel like `kunit_add_cleanup()` probably works better for a unit
> test framework.
> I think `kunit_remove_cleanup()` is fine and `kunit_release_cleanup()`
> is questionably ok.
> Instead of `release`, maybe it should be `kunit_trigger_cleanup()` or
> more verbosely, something like `kunit_early_trigger_cleanup()`.

Hmm... While personally I prefer 'defer' or 'cleanup' to 'action' in
isolation, I think the benefits of matching the devm_ API probably
exceed the benefits of a slightly better name here.

I'm less convinced by the _release_action() and _remove_action()
names: I definitely think 'trigger' is more obvious here. I hope that,
with some extra documentation, we can nevertheless make this
consistent with devm_*, but it's definitely suboptimal.

>
> I tried to look for equivalents in other languages/frameworks:
> * Rust and C++ rely on RAII, don't think they have equivalents in testing libs
> * Python has `self.addCleanup()`,
> https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
> * Go has `t.Cleanup()`, https://pkg.go.dev/testing#T.Cleanup
> * Looking at Zig since it also has a `defer`, I guess they just use
> that, I don't see anything in
> https://ziglang.org/documentation/master/std/#A;std:testing
> * I know nothing about JUnit, but a quick search seems like they rely
> on @After and @AfterClass annotations,
> https://junit.org/junit4/javadoc/4.12/org/junit/After.html
> * I know even less about HUnit, but it looks like it relies on
> wrapping things via the IO monad,
> https://hackage.haskell.org/package/HUnit-1.6.2.0/docs/Test-HUnit-Base.html#t:AssertionPredicate
> * Since we were inspired by TAP, I tried to look at Perl, but didn't
> immediately see anything that looked equivalent,
> https://metacpan.org/pod/Test::Most
>

Thanks for putting that together. It looks like cleanup is the winner
here, followed maybe by defer. I'd been using 'cleanup' to refer to
the sum total of all deferred functions, resource free functions, and
the test 'exit()' function (i.e., everything which runs after a failed
assertion), so I don't want to totally confuse the issue.

Regardless, it's probably worth at least having a mention in the
documentation that these are referred to as a cleanup in
Python/Go/etc, and are vaguely equivalent to 'defer' in Go and Zig.

> >
> > This is implemented as a resource under the hood, so the ordering
> > between resource cleanup and deferred functions is maintained.
> >
> > Signed-off-by: David Gow <davidgow@xxxxxxxxxx>
> > ---
>
> <snip>
>
> > diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> > index c0d88b318e90..6db28cd43e9b 100644
> > --- a/include/kunit/resource.h
> > +++ b/include/kunit/resource.h
> > @@ -387,4 +387,80 @@ static inline int kunit_destroy_named_resource(struct kunit *test,
> >   */
> >  void kunit_remove_resource(struct kunit *test, struct kunit_resource *res);
> >
> > +
> > +/**
> > + * kunit_add_action() - Defer an 'action' (function call) until the test ends.
> > + * @test: Test case to associate the action with.
> > + * @func: The function to run on test exit
> > + * @ctx: Data passed into @func
> > + *
> > + * Defer the execution of a function until the test exits, either normally or
> > + * due to a failure.  @ctx is passed as additional context. All functions
> > + * registered with kunit_add_action() will execute in the opposite order to that
> > + * they were registered in.
> > + *
> > + * This is useful for cleaning up allocated memory and resources.
>
> Re renaming to kunit_add_cleanup(), I think this makes writing the
> comment easier.
>
> E.g.
> - kunit_add_action() - Defer an 'action' (function call) until the test ends.
> + kunit_add_cleanup() - Call a function when the test ends.
> + ...
> + This is useful for cleaning up allocated memory and resources.
>

Good point. I think we can probably use the better description here
even if we don't rename the function.

Cheers,
-- David

> Daniel

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature