Re: [RFC v4 08/17] kunit: test: add support for test abort
On Mon, 2019-03-25 at 15:32 -0700, Brendan Higgins wrote: > On Fri, Mar 22, 2019 at 12:11 AM Knut Omang wrote: > > On Thu, 2019-03-21 at 18:41 -0700, Brendan Higgins wrote: > > > On Thu, Mar 21, 2019 at 6:10 PM Frank Rowand > > > wrote: > > > > On 2/27/19 11:42 PM, Brendan Higgins wrote: > > > > > On Tue, Feb 19, 2019 at 10:44 PM Frank Rowand > > > > > wrote: > > > > > > On 2/19/19 7:39 PM, Brendan Higgins wrote: > > > > > > > On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand > > > > > > > > > > > > > > wrote: > > > > > > > > On 2/14/19 1:37 PM, Brendan Higgins wrote: > < snip > > > > > > > > > kunit_abort() is what will be call as the result of an assert > > > > > > > > failure. > > > > > > > > > > > > > > Yep. Does that need clarified somewhere. > > > > > > > > BUG(), which is a panic, which is crashing the system is not > > > > > > > > acceptable > > > > > > > > in the Linux kernel. You will just annoy Linus if you submit > > > > > > > > this. > > > > > > > > > > > > > > Sorry, I thought this was an acceptable use case since, a) this > > > > > > > should > > > > > > > never be compiled in a production kernel, b) we are in a pretty > > > > > > > bad, > > > > > > > unpredictable state if we get here and keep going. I think you > > > > > > > might > > > > > > > have said elsewhere that you think "a" is not valid? In any case, > > > > > > > I > > > > > > > can replace this with a WARN, would that be acceptable? > > > > > > > > > > > > A WARN may or may not make sense, depending on the context. It may > > > > > > be sufficient to simply report a test failure (as in the old version > > > > > > of case (2) below. > > > > > > > > > > > > Answers to "a)" and "b)": > > > > > > > > > > > > a) it might be in a production kernel > > > > > > > > > > Sorry for a possibly stupid question, how might it be so? Why would > > > > > someone intentionally build unit tests into a production kernel? > > > > > > > > People do things. Just expect it. > > > > > > Huh, alright. I will take your word for it then. > > > > I have a better explanation: Production kernels have bugs, unfortunately. > > And sometimes those need to be investigated on systems than cannot be > > brought down or affected more than absolutely necessary, maybe via a third > > party > > doing the execution. A light weight, precise test (well tested ahead :) ) > > might > > be a way of proving or disproving assumptions that can lead to the > > development > > and application of a fix. > > Sorry, you are not suggesting testing in production are you? To be > clear, I am not concerned about someone using testing, KUnit, or > whatever in a *production-like* environment: that's not what we are > talking about here. My assumption is that no one will deploy tests > into actual production. And my take is that you should not make such assumptions. Even the cost of bringing down a "production-like" environment can be significant, and the test infrastructure shouldn't think of itself as important enough to justify doing such things. > > IMHO you're confusing "building into" with temporary applying, then removing > > again - like the difference between running a local user space program vs > > installing it under /usr and have it in everyone's PATH. > > I don't really see the point of distinguishing between "building into" > and "temporary applying" in this case; that's part of my point. Maybe > it makes sense in whitebox end-to-end testing, but in the case of unit > testing, I don't think so. > > > > > > > a') it is not acceptable in my development kernel either > > > > I think one of the fundamental properties of a good test framework is that > > it > > should not require changes to the code under test by itself. > > > > Sure, but that has nothing to do with the environment the code/tests > are running in. Well, just that if the tests require a special environment to run, you limit the usability of the tests in detecting or ruling out real issues. Thanks, Knut > > < snip > > > Cheers
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Thu, Feb 28, 2019 at 10:02 AM Stephen Boyd wrote:
>
> Quoting Brendan Higgins (2019-02-28 01:03:24)
> > On Tue, Feb 26, 2019 at 12:35 PM Stephen Boyd wrote:
> > >
> > > when they need to abort and then the test runner would detect that error
> > > via the return value from the 'run test' function. That would be a more
> > > direct approach, but also more verbose than a single KUNIT_ASSERT()
> > > line. It would be more kernel idiomatic too because the control flow
> >
> > Yeah, I was intentionally going against that idiom. I think that idiom
> > makes test logic more complicated than it needs to be, especially if
> > the assertion failure happens in a helper function; then you have to
> > pass that error all the way back up. It is important that test code
> > should be as simple as possible to the point of being immediately
> > obviously correct at first glance because there are no tests for
> > tests.
> >
> > The idea with assertions is that you use them to state all the
> > preconditions for your test. Logically speaking, these are the
> > premises of the test case, so if a premise isn't true, there is no
> > point in continuing the test case because there are no conclusions
> > that can be drawn without the premises. Whereas, the expectation is
> > the thing you are trying to prove. It is not used universally in
> > x-unit style test frameworks, but I really like it as a convention.
> > You could still express the idea of a premise using the above idiom,
> > but I think KUNIT_ASSERT_* states the intended idea perfectly.
>
> Fair enough. It would be great if these sorts of things were described
> in the commit text.
Good point. Will fix.
>
> Is the assumption that things like held locks and refcounted elements
> won't exist when one of these assertions is made? It sounds like some of
> the cleanup logic could be fairly complicated if a helper function
> changes some state and then an assert fails and we have to unwind all
> the state from a corrupt location. A similar problem exists for a test
> timeout too. How do we get back to a sane state if the test locks up for
> a long time? Just don't try?
It depends on the situation, if it is part of a KUnit test itself
(probably not code under test), then you can use the kunit_resource
API: https://lkml.org/lkml/2019/2/14/1125; it is inspired by the
devm_* family of functions, such that when a KUnit test case ends, for
any reason, all the kunit_resources are automatically cleaned up.
Similarly, kunit_module.exit is called at the end of every test case,
regardless of how it terminates.
>
> >
> > > isn't hidden inside a macro and it isn't intimately connected with
> > > kthreads and completions.
> >
> > Yeah, I wasn't a fan of that myself, but it was broadly available. My
> > previous version (still the architecture specific version for UML, not
> > in this patchset though) relies on UML_LONGJMP, but is obviously only
> > works on UML. A number of people wanted support for other
> > architectures. Rob and Luis specifically wanted me to provide a
> > version of abort that would work on any architecture, even if it only
> > had reduced functionality; I thought this fit the bill okay.
>
> Ok.
>
> >
> > >
> > > >
> > > > diff --git a/kunit/test.c b/kunit/test.c
> > > > index d18c50d5ed671..6e5244642ab07 100644
> > > > --- a/kunit/test.c
> > > > +++ b/kunit/test.c
> > > [...]
> > > > +
> > > > +static void kunit_generic_throw(struct kunit_try_catch *try_catch)
> > > > +{
> > > > + try_catch->context.try_result = -EFAULT;
> > > > + complete_and_exit(try_catch->context.try_completion, -EFAULT);
> > > > +}
> > > > +
> > > > +static int kunit_generic_run_threadfn_adapter(void *data)
> > > > +{
> > > > + struct kunit_try_catch *try_catch = data;
> > > >
> > > > + try_catch->try(&try_catch->context);
> > > > +
> > > > + complete_and_exit(try_catch->context.try_completion, 0);
> > >
> > > The exit code doesn't matter, right? If so, it might be clearer to just
> > > return 0 from this function because kthreads exit themselves as far as I
> > > recall.
> >
> > You mean complete and then return?
>
> Yes. I was confused for a minute because I thought the exit code was
> checked, but it isn't. Instead, the try_catch->context.try_result is
> where the test result goes, so calling exit explicitly doesn't seem to
> be important here, but it is important in the throw case.
Yep.
>
> >
> > >
> > > > + else if (exit_code)
> > > > + kunit_err(test, "Unknown error: %d", exit_code);
> > > > +}
> > > > +
> > > > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch)
> > > > +{
> > > > + try_catch->run = kunit_generic_run_try_catch;
> > >
> > > Is the idea that 'run' would be anything besides
> > > 'kunit_generic_run_try_catch'? If it isn't going to be different, then
> >
> > Yeah, it can be overridden with an architecture specific version.
> >
> > > maybe it's simpler to just have a function like
> > > kunit_g
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Thu, Feb 28, 2019 at 5:55 AM Dan Carpenter wrote:
>
> On Thu, Feb 28, 2019 at 01:03:24AM -0800, Brendan Higgins wrote:
> > you could do:
> >
> > if (IS_ERR_OR_NULL(ptr)) {
> > KUNIT_FAIL(test, "ptr is an errno or null: %ld", ptr);
> > return;
> > }
>
> It's best to not mix error pointers and NULL but when we do mix them,
> it means that NULL is a special kind of success. Like we try to load
> a feature and we get back:
>
> valid pointer <-- success
> null <-- feature is disabled. not an error.
> error pointer <-- feature is broken. fail.
Thanks for pointing that out! Will fix.
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Thu, Feb 28, 2019 at 01:03:24AM -0800, Brendan Higgins wrote:
> you could do:
>
> if (IS_ERR_OR_NULL(ptr)) {
> KUNIT_FAIL(test, "ptr is an errno or null: %ld", ptr);
> return;
> }
It's best to not mix error pointers and NULL but when we do mix them,
it means that NULL is a special kind of success. Like we try to load
a feature and we get back:
valid pointer <-- success
null <-- feature is disabled. not an error.
error pointer <-- feature is broken. fail.
regards,
dan carpenter
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Tue, Feb 26, 2019 at 12:35 PM Stephen Boyd wrote:
>
> Quoting Brendan Higgins (2019-02-14 13:37:20)
> > Add support for aborting/bailing out of test cases. Needed for
> > implementing assertions.
>
> Can you add some more text here with the motivating reasons for
> implementing assertions and bailing out of test cases?
Sure. Yeah, this comes before the commit that adds assertions, so I
should probably put a better explanation here.
>
> For example, I wonder why unit tests can't just return with a failure
Well, you could. You can just do the check as you would without KUnit,
except call KUNIT_FAIL(...) before you return. For example, instead
of:
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
you could do:
if (IS_ERR_OR_NULL(ptr)) {
KUNIT_FAIL(test, "ptr is an errno or null: %ld", ptr);
return;
}
> when they need to abort and then the test runner would detect that error
> via the return value from the 'run test' function. That would be a more
> direct approach, but also more verbose than a single KUNIT_ASSERT()
> line. It would be more kernel idiomatic too because the control flow
Yeah, I was intentionally going against that idiom. I think that idiom
makes test logic more complicated than it needs to be, especially if
the assertion failure happens in a helper function; then you have to
pass that error all the way back up. It is important that test code
should be as simple as possible to the point of being immediately
obviously correct at first glance because there are no tests for
tests.
The idea with assertions is that you use them to state all the
preconditions for your test. Logically speaking, these are the
premises of the test case, so if a premise isn't true, there is no
point in continuing the test case because there are no conclusions
that can be drawn without the premises. Whereas, the expectation is
the thing you are trying to prove. It is not used universally in
x-unit style test frameworks, but I really like it as a convention.
You could still express the idea of a premise using the above idiom,
but I think KUNIT_ASSERT_* states the intended idea perfectly.
> isn't hidden inside a macro and it isn't intimately connected with
> kthreads and completions.
Yeah, I wasn't a fan of that myself, but it was broadly available. My
previous version (still the architecture specific version for UML, not
in this patchset though) relies on UML_LONGJMP, but is obviously only
works on UML. A number of people wanted support for other
architectures. Rob and Luis specifically wanted me to provide a
version of abort that would work on any architecture, even if it only
had reduced functionality; I thought this fit the bill okay.
>
> >
> > Signed-off-by: Brendan Higgins
> [...]
> > diff --git a/kunit/test-test.c b/kunit/test-test.c
> > new file mode 100644
> > index 0..a936c041f1c8f
>
> Could this whole file be another patch? It seems to be a test for the
> try catch mechanism.
Sure.
>
> > diff --git a/kunit/test.c b/kunit/test.c
> > index d18c50d5ed671..6e5244642ab07 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> [...]
> > +
> > +static void kunit_generic_throw(struct kunit_try_catch *try_catch)
> > +{
> > + try_catch->context.try_result = -EFAULT;
> > + complete_and_exit(try_catch->context.try_completion, -EFAULT);
> > +}
> > +
> > +static int kunit_generic_run_threadfn_adapter(void *data)
> > +{
> > + struct kunit_try_catch *try_catch = data;
> >
> > + try_catch->try(&try_catch->context);
> > +
> > + complete_and_exit(try_catch->context.try_completion, 0);
>
> The exit code doesn't matter, right? If so, it might be clearer to just
> return 0 from this function because kthreads exit themselves as far as I
> recall.
You mean complete and then return?
>
> > +}
> > +
> > +static void kunit_generic_run_try_catch(struct kunit_try_catch *try_catch)
> > +{
> > + struct task_struct *task_struct;
> > + struct kunit *test = try_catch->context.test;
> > + int exit_code, wake_result;
> > + DECLARE_COMPLETION(test_case_completion);
>
> DECLARE_COMPLETION_ONSTACK()?
Whoops, yeah, that one.
>
> > +
> > + try_catch->context.try_completion = &test_case_completion;
> > + try_catch->context.try_result = 0;
> > + task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> > +try_catch,
> > +"kunit_try_catch_thread");
> > + if (IS_ERR_OR_NULL(task_struct)) {
>
> It looks like NULL is never returned from kthread_create(), so don't
> check for it here.
Bad habit, sorry.
>
> > + try_catch->catch(&try_catch->context);
> > + return;
> > + }
> > +
> > + wake_result = wake_up_process(task_struct);
> > + if (wake_result != 0 && wake_result != 1) {
>
> These are the only two possible return values of wake_up_process(), so
> why not just use kthread_run() and check for an error o
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Tue, Feb 19, 2019 at 10:44 PM Frank Rowand wrote:
>
> On 2/19/19 7:39 PM, Brendan Higgins wrote:
> > On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand
> > wrote:
> >>
> >> On 2/14/19 1:37 PM, Brendan Higgins wrote:
> >>> Add support for aborting/bailing out of test cases. Needed for
> >>> implementing assertions.
> >>>
> >>> Signed-off-by: Brendan Higgins
> >>> ---
> >>> Changes Since Last Version
> >>> - This patch is new introducing a new cross-architecture way to abort
> >>>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
> >>>details).
> >>> - On a side note, this is not a complete replacement for the UML abort
> >>>mechanism, but covers the majority of necessary functionality. UML
> >>>architecture specific featurs have been dropped from the initial
> >>>patchset.
> >>> ---
> >>> include/kunit/test.h | 24 +
> >>> kunit/Makefile | 3 +-
> >>> kunit/test-test.c| 127 ++
> >>> kunit/test.c | 208 +--
> >>> 4 files changed, 353 insertions(+), 9 deletions(-)
> >>> create mode 100644 kunit/test-test.c
> >>
> >> < snip >
> >>
> >>> diff --git a/kunit/test.c b/kunit/test.c
> >>> index d18c50d5ed671..6e5244642ab07 100644
> >>> --- a/kunit/test.c
> >>> +++ b/kunit/test.c
> >>> @@ -6,9 +6,9 @@
> >>> * Author: Brendan Higgins
> >>> */
> >>>
> >>> -#include
> >>> #include
> >>> -#include
> >>> +#include
> >>> +#include
> >>> #include
> >>>
> >>> static bool kunit_get_success(struct kunit *test)
> >>> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool
> >>> success)
> >>> spin_unlock_irqrestore(&test->lock, flags);
> >>> }
> >>>
> >>> +static bool kunit_get_death_test(struct kunit *test)
> >>> +{
> >>> + unsigned long flags;
> >>> + bool death_test;
> >>> +
> >>> + spin_lock_irqsave(&test->lock, flags);
> >>> + death_test = test->death_test;
> >>> + spin_unlock_irqrestore(&test->lock, flags);
> >>> +
> >>> + return death_test;
> >>> +}
> >>> +
> >>> +static void kunit_set_death_test(struct kunit *test, bool death_test)
> >>> +{
> >>> + unsigned long flags;
> >>> +
> >>> + spin_lock_irqsave(&test->lock, flags);
> >>> + test->death_test = death_test;
> >>> + spin_unlock_irqrestore(&test->lock, flags);
> >>> +}
> >>> +
> >>> static int kunit_vprintk_emit(const struct kunit *test,
> >>> int level,
> >>> const char *fmt,
> >>> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct
> >>> kunit_stream *stream)
> >>> stream->commit(stream);
> >>> }
> >>>
> >>> +static void __noreturn kunit_abort(struct kunit *test)
> >>> +{
> >>> + kunit_set_death_test(test, true);
> >>> +
> >>> + test->try_catch.throw(&test->try_catch);
> >>> +
> >>> + /*
> >>> + * Throw could not abort from test.
> >>> + */
> >>> + kunit_err(test, "Throw could not abort from test!");
> >>> + show_stack(NULL, NULL);
> >>> + BUG();
> >>
> >> kunit_abort() is what will be call as the result of an assert failure.
> >
> > Yep. Does that need clarified somewhere.
> >>
> >> BUG(), which is a panic, which is crashing the system is not acceptable
> >> in the Linux kernel. You will just annoy Linus if you submit this.
> >
> > Sorry, I thought this was an acceptable use case since, a) this should
> > never be compiled in a production kernel, b) we are in a pretty bad,
> > unpredictable state if we get here and keep going. I think you might
> > have said elsewhere that you think "a" is not valid? In any case, I
> > can replace this with a WARN, would that be acceptable?
>
> A WARN may or may not make sense, depending on the context. It may
> be sufficient to simply report a test failure (as in the old version
> of case (2) below.
>
> Answers to "a)" and "b)":
>
> a) it might be in a production kernel
Sorry for a possibly stupid question, how might it be so? Why would
someone intentionally build unit tests into a production kernel?
>
> a') it is not acceptable in my development kernel either
Fair enough.
>
> b) No. You don't crash a developer's kernel either unless it is
> required to avoid data corruption.
Alright, I thought that was one of those cases, but I am not going to
push the point. Also, in case it wasn't clear, the path where BUG()
gets called only happens if there is a bug in KUnit itself, not just
because a test case fails catastrophically.
>
> b') And you can not do replacements like:
>
> (1) in of_unittest_check_tree_linkage()
>
> - old -
>
> if (!of_root)
> return;
>
> - new -
>
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
>
> (2) in of_unittest_property_string()
>
> - old -
>
> /* of_property_read_string_index() tests */
> rc = of_property_read_string_index(np, "string-property", 0, strings);
> unittest(rc == 0 && !strcmp(string
Re: [RFC v4 08/17] kunit: test: add support for test abort
Quoting Brendan Higgins (2019-02-14 13:37:20)
> Add support for aborting/bailing out of test cases. Needed for
> implementing assertions.
Can you add some more text here with the motivating reasons for
implementing assertions and bailing out of test cases?
For example, I wonder why unit tests can't just return with a failure
when they need to abort and then the test runner would detect that error
via the return value from the 'run test' function. That would be a more
direct approach, but also more verbose than a single KUNIT_ASSERT()
line. It would be more kernel idiomatic too because the control flow
isn't hidden inside a macro and it isn't intimately connected with
kthreads and completions.
>
> Signed-off-by: Brendan Higgins
[...]
> diff --git a/kunit/test-test.c b/kunit/test-test.c
> new file mode 100644
> index 0..a936c041f1c8f
Could this whole file be another patch? It seems to be a test for the
try catch mechanism.
> diff --git a/kunit/test.c b/kunit/test.c
> index d18c50d5ed671..6e5244642ab07 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
[...]
> +
> +static void kunit_generic_throw(struct kunit_try_catch *try_catch)
> +{
> + try_catch->context.try_result = -EFAULT;
> + complete_and_exit(try_catch->context.try_completion, -EFAULT);
> +}
> +
> +static int kunit_generic_run_threadfn_adapter(void *data)
> +{
> + struct kunit_try_catch *try_catch = data;
>
> + try_catch->try(&try_catch->context);
> +
> + complete_and_exit(try_catch->context.try_completion, 0);
The exit code doesn't matter, right? If so, it might be clearer to just
return 0 from this function because kthreads exit themselves as far as I
recall.
> +}
> +
> +static void kunit_generic_run_try_catch(struct kunit_try_catch *try_catch)
> +{
> + struct task_struct *task_struct;
> + struct kunit *test = try_catch->context.test;
> + int exit_code, wake_result;
> + DECLARE_COMPLETION(test_case_completion);
DECLARE_COMPLETION_ONSTACK()?
> +
> + try_catch->context.try_completion = &test_case_completion;
> + try_catch->context.try_result = 0;
> + task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> +try_catch,
> +"kunit_try_catch_thread");
> + if (IS_ERR_OR_NULL(task_struct)) {
It looks like NULL is never returned from kthread_create(), so don't
check for it here.
> + try_catch->catch(&try_catch->context);
> + return;
> + }
> +
> + wake_result = wake_up_process(task_struct);
> + if (wake_result != 0 && wake_result != 1) {
These are the only two possible return values of wake_up_process(), so
why not just use kthread_run() and check for an error on the process
creation?
> + kunit_err(test, "task was not woken properly: %d",
> wake_result);
> + try_catch->catch(&try_catch->context);
> + }
> +
> + /*
> +* TODO(brendanhigg...@google.com): We should probably have some type
> of
> +* timeout here. The only question is what that timeout value should
> be.
> +*
> +* The intention has always been, at some point, to be able to label
> +* tests with some type of size bucket (unit/small,
> integration/medium,
> +* large/system/end-to-end, etc), where each size bucket would get a
> +* default timeout value kind of like what Bazel does:
> +*
> https://docs.bazel.build/versions/master/be/common-definitions.html#test.size
> +* There is still some debate to be had on exactly how we do this.
> (For
> +* one, we probably want to have some sort of test runner level
> +* timeout.)
> +*
> +* For more background on this topic, see:
> +* https://mike-bland.com/2011/11/01/small-medium-large.html
> +*/
> + wait_for_completion(&test_case_completion);
It doesn't seem like a bad idea to make this have some arbitrarily large
timeout value to start with. Just to make sure the whole thing doesn't
get wedged. It's a timeout, so in theory it should never trigger if it's
large enough.
> +
> + exit_code = try_catch->context.try_result;
> + if (exit_code == -EFAULT)
> + try_catch->catch(&try_catch->context);
> + else if (exit_code == -EINTR)
> + kunit_err(test, "wake_up_process() was never called.");
Does kunit_err() add newlines? It would be good to stick to the rest of
kernel style (printk, tracing, etc.) and rely on the callers to have the
newlines they want. Also, remove the full-stop because it isn't really
necessary to have those in error logs.
> + else if (exit_code)
> + kunit_err(test, "Unknown error: %d", exit_code);
> +}
> +
> +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch)
> +{
> + try_catch->run = kunit_generic_run_try_catch;
Is the idea that 'run' would be
Re: [RFC v4 08/17] kunit: test: add support for test abort
On 2/19/19 7:39 PM, Brendan Higgins wrote:
> On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand wrote:
>>
>> On 2/14/19 1:37 PM, Brendan Higgins wrote:
>>> Add support for aborting/bailing out of test cases. Needed for
>>> implementing assertions.
>>>
>>> Signed-off-by: Brendan Higgins
>>> ---
>>> Changes Since Last Version
>>> - This patch is new introducing a new cross-architecture way to abort
>>>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
>>>details).
>>> - On a side note, this is not a complete replacement for the UML abort
>>>mechanism, but covers the majority of necessary functionality. UML
>>>architecture specific featurs have been dropped from the initial
>>>patchset.
>>> ---
>>> include/kunit/test.h | 24 +
>>> kunit/Makefile | 3 +-
>>> kunit/test-test.c| 127 ++
>>> kunit/test.c | 208 +--
>>> 4 files changed, 353 insertions(+), 9 deletions(-)
>>> create mode 100644 kunit/test-test.c
>>
>> < snip >
>>
>>> diff --git a/kunit/test.c b/kunit/test.c
>>> index d18c50d5ed671..6e5244642ab07 100644
>>> --- a/kunit/test.c
>>> +++ b/kunit/test.c
>>> @@ -6,9 +6,9 @@
>>> * Author: Brendan Higgins
>>> */
>>>
>>> -#include
>>> #include
>>> -#include
>>> +#include
>>> +#include
>>> #include
>>>
>>> static bool kunit_get_success(struct kunit *test)
>>> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool
>>> success)
>>> spin_unlock_irqrestore(&test->lock, flags);
>>> }
>>>
>>> +static bool kunit_get_death_test(struct kunit *test)
>>> +{
>>> + unsigned long flags;
>>> + bool death_test;
>>> +
>>> + spin_lock_irqsave(&test->lock, flags);
>>> + death_test = test->death_test;
>>> + spin_unlock_irqrestore(&test->lock, flags);
>>> +
>>> + return death_test;
>>> +}
>>> +
>>> +static void kunit_set_death_test(struct kunit *test, bool death_test)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&test->lock, flags);
>>> + test->death_test = death_test;
>>> + spin_unlock_irqrestore(&test->lock, flags);
>>> +}
>>> +
>>> static int kunit_vprintk_emit(const struct kunit *test,
>>> int level,
>>> const char *fmt,
>>> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct
>>> kunit_stream *stream)
>>> stream->commit(stream);
>>> }
>>>
>>> +static void __noreturn kunit_abort(struct kunit *test)
>>> +{
>>> + kunit_set_death_test(test, true);
>>> +
>>> + test->try_catch.throw(&test->try_catch);
>>> +
>>> + /*
>>> + * Throw could not abort from test.
>>> + */
>>> + kunit_err(test, "Throw could not abort from test!");
>>> + show_stack(NULL, NULL);
>>> + BUG();
>>
>> kunit_abort() is what will be call as the result of an assert failure.
>
> Yep. Does that need clarified somewhere.
>>
>> BUG(), which is a panic, which is crashing the system is not acceptable
>> in the Linux kernel. You will just annoy Linus if you submit this.
>
> Sorry, I thought this was an acceptable use case since, a) this should
> never be compiled in a production kernel, b) we are in a pretty bad,
> unpredictable state if we get here and keep going. I think you might
> have said elsewhere that you think "a" is not valid? In any case, I
> can replace this with a WARN, would that be acceptable?
A WARN may or may not make sense, depending on the context. It may
be sufficient to simply report a test failure (as in the old version
of case (2) below.
Answers to "a)" and "b)":
a) it might be in a production kernel
a') it is not acceptable in my development kernel either
b) No. You don't crash a developer's kernel either unless it is
required to avoid data corruption.
b') And you can not do replacements like:
(1) in of_unittest_check_tree_linkage()
- old -
if (!of_root)
return;
- new -
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
(2) in of_unittest_property_string()
- old -
/* of_property_read_string_index() tests */
rc = of_property_read_string_index(np, "string-property", 0, strings);
unittest(rc == 0 && !strcmp(strings[0], "foobar"),
"of_property_read_string_index() failure; rc=%i\n", rc);
- new -
/* of_property_read_string_index() tests */
rc = of_property_read_string_index(np, "string-property", 0, strings);
KUNIT_ASSERT_EQ(test, rc, 0);
KUNIT_EXPECT_STREQ(test, strings[0], "foobar");
If a test fails, that is no reason to abort testing. The remainder of the unit
tests can still run. There may be cascading failures, but that is ok.
Re: [RFC v4 08/17] kunit: test: add support for test abort
On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand wrote:
>
> On 2/14/19 1:37 PM, Brendan Higgins wrote:
> > Add support for aborting/bailing out of test cases. Needed for
> > implementing assertions.
> >
> > Signed-off-by: Brendan Higgins
> > ---
> > Changes Since Last Version
> > - This patch is new introducing a new cross-architecture way to abort
> >out of a test case (needed for KUNIT_ASSERT_*, see next patch for
> >details).
> > - On a side note, this is not a complete replacement for the UML abort
> >mechanism, but covers the majority of necessary functionality. UML
> >architecture specific featurs have been dropped from the initial
> >patchset.
> > ---
> > include/kunit/test.h | 24 +
> > kunit/Makefile | 3 +-
> > kunit/test-test.c| 127 ++
> > kunit/test.c | 208 +--
> > 4 files changed, 353 insertions(+), 9 deletions(-)
> > create mode 100644 kunit/test-test.c
>
> < snip >
>
> > diff --git a/kunit/test.c b/kunit/test.c
> > index d18c50d5ed671..6e5244642ab07 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> > @@ -6,9 +6,9 @@
> > * Author: Brendan Higgins
> > */
> >
> > -#include
> > #include
> > -#include
> > +#include
> > +#include
> > #include
> >
> > static bool kunit_get_success(struct kunit *test)
> > @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool
> > success)
> > spin_unlock_irqrestore(&test->lock, flags);
> > }
> >
> > +static bool kunit_get_death_test(struct kunit *test)
> > +{
> > + unsigned long flags;
> > + bool death_test;
> > +
> > + spin_lock_irqsave(&test->lock, flags);
> > + death_test = test->death_test;
> > + spin_unlock_irqrestore(&test->lock, flags);
> > +
> > + return death_test;
> > +}
> > +
> > +static void kunit_set_death_test(struct kunit *test, bool death_test)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&test->lock, flags);
> > + test->death_test = death_test;
> > + spin_unlock_irqrestore(&test->lock, flags);
> > +}
> > +
> > static int kunit_vprintk_emit(const struct kunit *test,
> > int level,
> > const char *fmt,
> > @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct
> > kunit_stream *stream)
> > stream->commit(stream);
> > }
> >
> > +static void __noreturn kunit_abort(struct kunit *test)
> > +{
> > + kunit_set_death_test(test, true);
> > +
> > + test->try_catch.throw(&test->try_catch);
> > +
> > + /*
> > + * Throw could not abort from test.
> > + */
> > + kunit_err(test, "Throw could not abort from test!");
> > + show_stack(NULL, NULL);
> > + BUG();
>
> kunit_abort() is what will be call as the result of an assert failure.
Yep. Does that need clarified somewhere?
>
> BUG(), which is a panic, which is crashing the system is not acceptable
> in the Linux kernel. You will just annoy Linus if you submit this.
Sorry, I thought this was an acceptable use case since, a) this should
never be compiled in a production kernel, b) we are in a pretty bad,
unpredictable state if we get here and keep going. I think you might
have said elsewhere that you think "a" is not valid? In any case, I
can replace this with a WARN, would that be acceptable?
Re: [RFC v4 08/17] kunit: test: add support for test abort
On 2/14/19 1:37 PM, Brendan Higgins wrote:
> Add support for aborting/bailing out of test cases. Needed for
> implementing assertions.
>
> Signed-off-by: Brendan Higgins
> ---
> Changes Since Last Version
> - This patch is new introducing a new cross-architecture way to abort
>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
>details).
> - On a side note, this is not a complete replacement for the UML abort
>mechanism, but covers the majority of necessary functionality. UML
>architecture specific featurs have been dropped from the initial
>patchset.
> ---
> include/kunit/test.h | 24 +
> kunit/Makefile | 3 +-
> kunit/test-test.c| 127 ++
> kunit/test.c | 208 +--
> 4 files changed, 353 insertions(+), 9 deletions(-)
> create mode 100644 kunit/test-test.c
< snip >
> diff --git a/kunit/test.c b/kunit/test.c
> index d18c50d5ed671..6e5244642ab07 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -6,9 +6,9 @@
> * Author: Brendan Higgins
> */
>
> -#include
> #include
> -#include
> +#include
> +#include
> #include
>
> static bool kunit_get_success(struct kunit *test)
> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool
> success)
> spin_unlock_irqrestore(&test->lock, flags);
> }
>
> +static bool kunit_get_death_test(struct kunit *test)
> +{
> + unsigned long flags;
> + bool death_test;
> +
> + spin_lock_irqsave(&test->lock, flags);
> + death_test = test->death_test;
> + spin_unlock_irqrestore(&test->lock, flags);
> +
> + return death_test;
> +}
> +
> +static void kunit_set_death_test(struct kunit *test, bool death_test)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&test->lock, flags);
> + test->death_test = death_test;
> + spin_unlock_irqrestore(&test->lock, flags);
> +}
> +
> static int kunit_vprintk_emit(const struct kunit *test,
> int level,
> const char *fmt,
> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct
> kunit_stream *stream)
> stream->commit(stream);
> }
>
> +static void __noreturn kunit_abort(struct kunit *test)
> +{
> + kunit_set_death_test(test, true);
> +
> + test->try_catch.throw(&test->try_catch);
> +
> + /*
> + * Throw could not abort from test.
> + */
> + kunit_err(test, "Throw could not abort from test!");
> + show_stack(NULL, NULL);
> + BUG();
kunit_abort() is what will be call as the result of an assert failure.
BUG(), which is a panic, which is crashing the system is not acceptable
in the Linux kernel. You will just annoy Linus if you submit this.
-Frank
< snip >
[RFC v4 08/17] kunit: test: add support for test abort
Add support for aborting/bailing out of test cases. Needed for
implementing assertions.
Signed-off-by: Brendan Higgins
---
Changes Since Last Version
- This patch is new introducing a new cross-architecture way to abort
out of a test case (needed for KUNIT_ASSERT_*, see next patch for
details).
- On a side note, this is not a complete replacement for the UML abort
mechanism, but covers the majority of necessary functionality. UML
architecture specific featurs have been dropped from the initial
patchset.
---
include/kunit/test.h | 24 +
kunit/Makefile | 3 +-
kunit/test-test.c| 127 ++
kunit/test.c | 208 +--
4 files changed, 353 insertions(+), 9 deletions(-)
create mode 100644 kunit/test-test.c
diff --git a/include/kunit/test.h b/include/kunit/test.h
index a36ad1a502c66..cd02dca96eb61 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -151,6 +151,26 @@ struct kunit_module {
struct kunit_case *test_cases;
};
+struct kunit_try_catch_context {
+ struct kunit *test;
+ struct kunit_module *module;
+ struct kunit_case *test_case;
+ struct completion *try_completion;
+ int try_result;
+};
+
+struct kunit_try_catch {
+ void (*run)(struct kunit_try_catch *try_catch);
+ void (*throw)(struct kunit_try_catch *try_catch);
+ struct kunit_try_catch_context context;
+ void (*try)(struct kunit_try_catch_context *context);
+ void (*catch)(struct kunit_try_catch_context *context);
+};
+
+void kunit_try_catch_init(struct kunit_try_catch *try_catch);
+
+void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
+
/**
* struct kunit - represents a running instance of a test.
* @priv: for user to store arbitrary data. Commonly used to pass data created
@@ -166,13 +186,17 @@ struct kunit {
/* private: internal use only. */
const char *name; /* Read only after initialization! */
+ struct kunit_try_catch try_catch;
spinlock_t lock; /* Gaurds all mutable test state. */
bool success; /* Protected by lock. */
+ bool death_test; /* Protected by lock. */
struct list_head resources; /* Protected by lock. */
+ void (*set_death_test)(struct kunit *test, bool death_test);
void (*vprintk)(const struct kunit *test,
const char *level,
struct va_format *vaf);
void (*fail)(struct kunit *test, struct kunit_stream *stream);
+ void (*abort)(struct kunit *test);
};
int kunit_init_test(struct kunit *test, const char *name);
diff --git a/kunit/Makefile b/kunit/Makefile
index 60a9ea6cb4697..e4c300f67479a 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_KUNIT) += test.o \
string-stream.o \
kunit-stream.o
-obj-$(CONFIG_KUNIT_TEST) +=string-stream-test.o
+obj-$(CONFIG_KUNIT_TEST) +=test-test.o \
+ string-stream-test.o
obj-$(CONFIG_KUNIT_EXAMPLE_TEST) +=example-test.o
diff --git a/kunit/test-test.c b/kunit/test-test.c
new file mode 100644
index 0..a936c041f1c8f
--- /dev/null
+++ b/kunit/test-test.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for core test infrastructure.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins
+ */
+#include
+
+struct kunit_try_catch_test_context {
+ struct kunit_try_catch *try_catch;
+ bool function_called;
+};
+
+void kunit_test_successful_try(struct kunit_try_catch_context *context)
+{
+ struct kunit_try_catch_test_context *ctx = context->test->priv;
+
+ ctx->function_called = true;
+}
+
+void kunit_test_no_catch(struct kunit_try_catch_context *context)
+{
+ KUNIT_FAIL(context->test, "Catch should not be called.");
+}
+
+static void kunit_test_try_catch_successful_try_no_catch(struct kunit *test)
+{
+ struct kunit_try_catch_test_context *ctx = test->priv;
+ struct kunit_try_catch *try_catch = ctx->try_catch;
+
+ try_catch->try = kunit_test_successful_try;
+ try_catch->catch = kunit_test_no_catch;
+ try_catch->run(try_catch);
+
+ KUNIT_EXPECT_TRUE(test, ctx->function_called);
+}
+
+void kunit_test_unsuccessful_try(struct kunit_try_catch_context *context)
+{
+ struct kunit_try_catch *try_catch = container_of(context,
+struct kunit_try_catch,
+context);
+
+ try_catch->throw(try_catch);
+ KUNIT_FAIL(context->test, "This line should never be reached.");
+}
+
+void kunit_test_catch(struct kunit_try_catch_context *context)
+{
+ struct kunit_try_catch_test_context *ctx = context->test->priv;
+
+ ctx->function_called = true;
+}
+
+static v
