On Wed, 26 Apr 2023 at 10:12, Daniel Latypov dlatypov@google.com wrote:
On Fri, Apr 21, 2023 at 1:42 AM David Gow davidgow@google.com 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%3Bstd: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#...
- 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@google.com
<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