| Message ID | 20230421084226.2278282-2-davidgow@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: Deferred action helpers | expand |
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()`. 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 > > 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. Daniel
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;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@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
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. + * + * Returns: + * 0 on success, an error if the action could not be deferred. + */ +int kunit_add_action(struct kunit *test, void (*action) (void *), void *ctx); + +/** + * kunit_add_action_or_reset() - 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. + * + * If the action cannot be created (e.g., due to the system being out of memory), + * then action(ctx) will be called immediately, and an error will be returned. + * + * Returns: + * 0 on success, an error if the action could not be deferred. + */ +int kunit_add_action_or_reset(struct kunit *test, void (*action)(void *), + void *ctx); + +/** + * kunit_remove_action() - Cancel a matching deferred action. + * @test: Test case the action is associated with. + * @func: The deferred function to cancel. + * @ctx: The context passed to the deferred function to trigger. + * + * Prevent an action deferred via kunit_add_action() from executing when the + * test terminates.. + * If the function/context pair was deferred multiple times, only the most + * recent one will be cancelled. + */ +void kunit_remove_action(struct kunit *test, + void (*action)(void *), + void *ctx); + +/** + * kunit_release_action() - Run a matching action call immediately. + * @test: Test case the action is associated with. + * @func: The deferred function to trigger. + * @ctx: The context passed to the deferred function to trigger. + * + * Execute a function deferred via kunit_add_action()) immediately, rather than + * when the test ends. + * If the function/context pair was deferred multiple times, it will only be + * executed once here. The most recent deferral will no longer execute when + * the test ends. + * + * kunit_release_action(test, func, ctx); + * is equivalent to + * func(ctx); + * kunit_remove_action(test, func, ctx); + */ +void kunit_release_action(struct kunit *test, + void (*action)(void *), + void *ctx); #endif /* _KUNIT_RESOURCE_H */ diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 42e44caa1bdd..83d8e90ca7a2 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -112,7 +112,7 @@ struct kunit_test_resource_context { struct kunit test; bool is_resource_initialized; int allocate_order[2]; - int free_order[2]; + int free_order[4]; }; static int fake_resource_init(struct kunit_resource *res, void *context) @@ -403,6 +403,88 @@ static void kunit_resource_test_named(struct kunit *test) KUNIT_EXPECT_TRUE(test, list_empty(&test->resources)); } +static void increment_int(void *ctx) +{ + int *i = (int *)ctx; + (*i)++; +} + +static void kunit_resource_test_action(struct kunit *test) +{ + int num_actions = 0; + + kunit_add_action(test, increment_int, &num_actions); + KUNIT_EXPECT_EQ(test, num_actions, 0); + kunit_cleanup(test); + KUNIT_EXPECT_EQ(test, num_actions, 1); + + /* Once we've cleaned up, the action queue is empty. */ + kunit_cleanup(test); + KUNIT_EXPECT_EQ(test, num_actions, 1); + + /* Check the same function can be deferred multiple times. */ + kunit_add_action(test, increment_int, &num_actions); + kunit_add_action(test, increment_int, &num_actions); + kunit_cleanup(test); + KUNIT_EXPECT_EQ(test, num_actions, 3); +} +static void kunit_resource_test_remove_action(struct kunit *test) +{ + int num_actions = 0; + + kunit_add_action(test, increment_int, &num_actions); + KUNIT_EXPECT_EQ(test, num_actions, 0); + + kunit_remove_action(test, increment_int, &num_actions); + kunit_cleanup(test); + KUNIT_EXPECT_EQ(test, num_actions, 0); +} +static void kunit_resource_test_release_action(struct kunit *test) +{ + int num_actions = 0; + + kunit_add_action(test, increment_int, &num_actions); + KUNIT_EXPECT_EQ(test, num_actions, 0); + /* Runs immediately on trigger. */ + kunit_release_action(test, increment_int, &num_actions); + KUNIT_EXPECT_EQ(test, num_actions, 1); + + /* Doesn't run again on test exit. */ + kunit_cleanup(test); + KUNIT_EXPECT_EQ(test, num_actions, 1); +} +static void action_order_1(void *ctx) +{ + struct kunit_test_resource_context *res_ctx = (struct kunit_test_resource_context *)ctx; + + KUNIT_RESOURCE_TEST_MARK_ORDER(res_ctx, free_order, 1); + kunit_log(KERN_INFO, current->kunit_test, "action_order_1"); +} +static void action_order_2(void *ctx) +{ + struct kunit_test_resource_context *res_ctx = (struct kunit_test_resource_context *)ctx; + + KUNIT_RESOURCE_TEST_MARK_ORDER(res_ctx, free_order, 2); + kunit_log(KERN_INFO, current->kunit_test, "action_order_2"); +} +static void kunit_resource_test_action_ordering(struct kunit *test) +{ + struct kunit_test_resource_context *ctx = test->priv; + + kunit_add_action(test, action_order_1, ctx); + kunit_add_action(test, action_order_2, ctx); + kunit_add_action(test, action_order_1, ctx); + kunit_add_action(test, action_order_2, ctx); + kunit_remove_action(test, action_order_1, ctx); + kunit_release_action(test, action_order_2, ctx); + kunit_cleanup(test); + + /* [2 is triggered] [2], [(1 is cancelled)] [1] */ + KUNIT_EXPECT_EQ(test, ctx->free_order[0], 2); + KUNIT_EXPECT_EQ(test, ctx->free_order[1], 2); + KUNIT_EXPECT_EQ(test, ctx->free_order[2], 1); +} + static int kunit_resource_test_init(struct kunit *test) { struct kunit_test_resource_context *ctx = @@ -434,6 +516,10 @@ static struct kunit_case kunit_resource_test_cases[] = { KUNIT_CASE(kunit_resource_test_proper_free_ordering), KUNIT_CASE(kunit_resource_test_static), KUNIT_CASE(kunit_resource_test_named), + KUNIT_CASE(kunit_resource_test_action), + KUNIT_CASE(kunit_resource_test_remove_action), + KUNIT_CASE(kunit_resource_test_release_action), + KUNIT_CASE(kunit_resource_test_action_ordering), {} }; diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c index c414df922f34..20b08b9341c9 100644 --- a/lib/kunit/resource.c +++ b/lib/kunit/resource.c @@ -77,3 +77,98 @@ int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match, return 0; } EXPORT_SYMBOL_GPL(kunit_destroy_resource); + +struct kunit_action_ctx { + struct kunit_resource res; + void (*func)(void *); + void *ctx; +}; + +static void __kunit_action_free(struct kunit_resource *res) +{ + struct kunit_action_ctx *action_ctx = container_of(res, struct kunit_action_ctx, res); + + action_ctx->func(action_ctx->ctx); +} + + +int kunit_add_action(struct kunit *test, void (*action)(void *), void *ctx) +{ + struct kunit_action_ctx *action_ctx; + + KUNIT_ASSERT_NOT_NULL_MSG(test, action, "Tried to action a NULL function!"); + + action_ctx = kzalloc(sizeof(*action_ctx), GFP_KERNEL); + if (!action_ctx) + return -ENOMEM; + + action_ctx->func = action; + action_ctx->ctx = ctx; + + action_ctx->res.should_kfree = true; + /* As init is NULL, this cannot fail. */ + __kunit_add_resource(test, NULL, __kunit_action_free, &action_ctx->res, action_ctx); + + return 0; +} + +int kunit_add_action_or_reset(struct kunit *test, void (*action)(void *), + void *ctx) +{ + int res = kunit_add_action(test, action, ctx); + + if (res) + action(ctx); + return res; +} + +static bool __kunit_action_match(struct kunit *test, + struct kunit_resource *res, void *match_data) +{ + struct kunit_action_ctx *match_ctx = (struct kunit_action_ctx *)match_data; + struct kunit_action_ctx *res_ctx = container_of(res, struct kunit_action_ctx, res); + + /* Make sure this is a free function. */ + if (res->free != __kunit_action_free) + return false; + + /* Both the function and context data should match. */ + return (match_ctx->func == res_ctx->func) && (match_ctx->ctx == res_ctx->ctx); +} + +void kunit_remove_action(struct kunit *test, + void (*action)(void *), + void *ctx) +{ + struct kunit_action_ctx match_ctx; + struct kunit_resource *res; + + match_ctx.func = action; + match_ctx.ctx = ctx; + + res = kunit_find_resource(test, __kunit_action_match, &match_ctx); + if (res) { + /* Remove the free function so we don't run the action. */ + res->free = NULL; + kunit_remove_resource(test, res); + kunit_put_resource(res); + } +} + +void kunit_release_action(struct kunit *test, + void (*action)(void *), + void *ctx) +{ + struct kunit_action_ctx match_ctx; + struct kunit_resource *res; + + match_ctx.func = action; + match_ctx.ctx = ctx; + + res = kunit_find_resource(test, __kunit_action_match, &match_ctx); + if (res) { + kunit_remove_resource(test, res); + /* We have to put() this here, else free won't be called. */ + kunit_put_resource(res); + } +}
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. 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> --- Note: there is a checkpatch warning: "function definition argument 'void *' should also have an identifier name" I think this is a false-positive, as we're defining a function pointer struct member, not a specific function. Changes since RFC v2: https://lore.kernel.org/linux-kselftest/20230331080411.981038-2-davidgow@google.com/ - Got rid of internal_gfp - everything uses GFP_KERNEL now - This includes kunit_kzalloc() and friends, which still allocate the returned memory with the provided GFP, but use GFP_KERNEL for internal bookkeeping data. - Thanks Maxime & Benjamin! - Got rid of cancellation tokens. - kunit_add_action() now returns 0 on success, otherwise an error - Note that this can quite easily lead to a memory leak, so look at kunit_add_action_or_reset() - Thanks Maxime & Benjamin! - Added kunit_add_action_or_reset - Matches devm_add_action_or_reset() - Returns 0 on success. - Thanks Maxime & Benjamin! - Got rid of the kunit_defer_func_t typedef. - I liked it, but it is probably pushing the boundaries of kernel style. - Use (void (*)(void *)) instead. Changes since RFC v1: https://lore.kernel.org/linux-kselftest/20230325043104.3761770-2-davidgow@google.com/ - Rename functions to better match the devm_* APIs. (Thanks Maxime) - Embed the kunit_resource in struct kunit_action_ctx to avoid an extra allocation (Thanks Benjamin) - Use 'struct kunit_action_ctx' as the type for cancellation tokens (Thanks Benjamin) - Add tests. include/kunit/resource.h | 76 ++++++++++++++++++++++++++++++++ lib/kunit/kunit-test.c | 88 ++++++++++++++++++++++++++++++++++++- lib/kunit/resource.c | 95 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+), 1 deletion(-)