| Message ID | 20210526081112.3652290-3-davidgow@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/3] kunit: Support skipped tests | expand |
On Wed, May 26, 2021 at 01:11AM -0700, David Gow wrote: > Add a new KUnit test suite which contains tests which are always > skipped. This is used as an example for how to write tests which are > skipped, and to demonstrate the difference between kunit_skip() and > kunit_mark_skipped(). > > Because these tests do not pass (they're skipped), they are not enabled > by default, or by the KUNIT_ALL_TESTS config option: they must be > enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either > a .config or .kunitconfig file. > > Signed-off-by: David Gow <davidgow@google.com> > --- > lib/kunit/Kconfig | 15 +++++++++ > lib/kunit/Makefile | 2 ++ > lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+) > create mode 100644 lib/kunit/kunit-example-skip-test.c I don't know if this test is useful for a user of KUnit. Given it's not testing KUnit functionality (I see you added tests that the feature works in patch 1/3), but rather a demonstration and therefore dead code. I don't think the feature is difficult to understand from the API doc text. Instead, would it be more helpful to add something to Documentation/dev-tools/kunit? Or perhaps just add something to lib/kunit/kunit-example-test.c? It'd avoid introducing more Kconfig options at least. Thanks, -- Marco
On Wed, May 26, 2021 at 1:56 AM 'Marco Elver' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Wed, May 26, 2021 at 01:11AM -0700, David Gow wrote: > > Add a new KUnit test suite which contains tests which are always > > skipped. This is used as an example for how to write tests which are > > skipped, and to demonstrate the difference between kunit_skip() and > > kunit_mark_skipped(). > > > > Because these tests do not pass (they're skipped), they are not enabled > > by default, or by the KUNIT_ALL_TESTS config option: they must be > > enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either > > a .config or .kunitconfig file. > > > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > lib/kunit/Kconfig | 15 +++++++++ > > lib/kunit/Makefile | 2 ++ > > lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ > > 3 files changed, 69 insertions(+) > > create mode 100644 lib/kunit/kunit-example-skip-test.c > > I don't know if this test is useful for a user of KUnit. Given it's not > testing KUnit functionality (I see you added tests that the feature > works in patch 1/3), but rather a demonstration and therefore dead code. > I don't think the feature is difficult to understand from the API doc > text. > > Instead, would it be more helpful to add something to > Documentation/dev-tools/kunit? Or perhaps just add something to > lib/kunit/kunit-example-test.c? It'd avoid introducing more Kconfig I'm in favor of putting it in kunit-example-test.c as well. But I hear there was pushback to have a non-passing test in the example? I guess the fear is that someone will see something that doesn't say "passed" in the example output and think something has gone wrong? Hence this more conservative change. But I hope that in the absence of any replies in opposition, we can just keep one example-test.c > options at least. > > Thanks, > -- Marco > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/YK4NRlyrYJ8ktsWQ%40elver.google.com.
On Wed, May 26, 2021 at 11:29AM -0700, Daniel Latypov wrote: > On Wed, May 26, 2021 at 1:56 AM 'Marco Elver' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > On Wed, May 26, 2021 at 01:11AM -0700, David Gow wrote: > > > Add a new KUnit test suite which contains tests which are always > > > skipped. This is used as an example for how to write tests which are > > > skipped, and to demonstrate the difference between kunit_skip() and > > > kunit_mark_skipped(). > > > > > > Because these tests do not pass (they're skipped), they are not enabled > > > by default, or by the KUNIT_ALL_TESTS config option: they must be > > > enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either > > > a .config or .kunitconfig file. > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > --- > > > lib/kunit/Kconfig | 15 +++++++++ > > > lib/kunit/Makefile | 2 ++ > > > lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ > > > 3 files changed, 69 insertions(+) > > > create mode 100644 lib/kunit/kunit-example-skip-test.c > > > > I don't know if this test is useful for a user of KUnit. Given it's not > > testing KUnit functionality (I see you added tests that the feature > > works in patch 1/3), but rather a demonstration and therefore dead code. > > I don't think the feature is difficult to understand from the API doc > > text. > > > > Instead, would it be more helpful to add something to > > Documentation/dev-tools/kunit? Or perhaps just add something to > > lib/kunit/kunit-example-test.c? It'd avoid introducing more Kconfig > > I'm in favor of putting it in kunit-example-test.c as well. > > But I hear there was pushback to have a non-passing test in the example? > I guess the fear is that someone will see something that doesn't say > "passed" in the example output and think something has gone wrong? > > Hence this more conservative change. > But I hope that in the absence of any replies in opposition, we can > just keep one example-test.c Maybe I misunderstood, but kunit_skip*() isn't supposed to change the test ok/fail state, right? That's the behaviour I'd expect at least. So if the test case deliberately doesn't change the state, but just skips, it should be fine in example-test.c. Thanks, -- Marco
On Wed, May 26, 2021 at 1:11 AM 'David Gow' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Add a new KUnit test suite which contains tests which are always > skipped. This is used as an example for how to write tests which are > skipped, and to demonstrate the difference between kunit_skip() and > kunit_mark_skipped(). > > Because these tests do not pass (they're skipped), they are not enabled > by default, or by the KUNIT_ALL_TESTS config option: they must be > enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either > a .config or .kunitconfig file. > > Signed-off-by: David Gow <davidgow@google.com> Reviewed-by: Daniel Latypov <dlatypov@google.com> LGTM, two minor nits, and as noted elsewhere, I'd like it if we could have this be in the normal example test. But even if that doesn't happen, this seems fine to me. > --- > lib/kunit/Kconfig | 15 +++++++++ > lib/kunit/Makefile | 2 ++ > lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+) > create mode 100644 lib/kunit/kunit-example-skip-test.c > > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig > index 0b5dfb001bac..399fe5f789f7 100644 > --- a/lib/kunit/Kconfig > +++ b/lib/kunit/Kconfig > @@ -45,6 +45,21 @@ config KUNIT_EXAMPLE_TEST > is intended for curious hackers who would like to understand how to > use KUnit for kernel development. > > +config KUNIT_EXAMPLE_SKIP_TEST > + tristate "Skipped test example for KUnit" > + default n > + help > + Enables an example unit test that is always skipped. > + > + This test only exists to help new users understand what KUnit is and > + how it is used. Please refer to the example test itself, > + lib/kunit/example-test.c, for more information. This option is > + intended for curious hackers who would like to understand how to use > + KUnit for kernel development. > + > + Because this test does not pass, it is not enabled by > + CONFIG_KUNIT_ALL_TESTS > + > config KUNIT_ALL_TESTS > tristate "All KUnit tests with satisfied dependencies" > help > diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile > index c49f4ffb6273..8a99ff2f83bd 100644 > --- a/lib/kunit/Makefile > +++ b/lib/kunit/Makefile > @@ -18,3 +18,5 @@ obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o > endif > > obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o > + > +obj-$(CONFIG_KUNIT_EXAMPLE_SKIP_TEST) += kunit-example-skip-test.o > diff --git a/lib/kunit/kunit-example-skip-test.c b/lib/kunit/kunit-example-skip-test.c > new file mode 100644 > index 000000000000..5395ee0be485 > --- /dev/null > +++ b/lib/kunit/kunit-example-skip-test.c > @@ -0,0 +1,52 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Example KUnit test which is always skipped. > + * > + * Copyright (C) 2021, Google LLC. > + * Author: David Gow <davidgow@google.com> > + */ > + > +#include <kunit/test.h> > + > +/* > + * This test should always be skipped. > + */ > + > +static void example_skip_test(struct kunit *test) > +{ > + /* This line should run */ > + kunit_log(KERN_INFO, test, "You should not see a line below."); Btw, why not "kunit_info(test, ...)" ? > + > + /* Skip (and abort) the test */ > + kunit_skip(test, "this test should be skipped"); > + > + /* This line should not execute */ > + kunit_log(KERN_INFO, test, "You should not see this line."); Would it be more or less confusing to have KUNIT_FAIL(test, "We should not get to this line") maybe in addition or instead of this log? > +} > + > +static void example_mark_skipped_test(struct kunit *test) > +{ > + /* This line should run */ > + kunit_log(KERN_INFO, test, "You should see a line below."); > + > + /* Skip (but do not abort) the test */ > + kunit_mark_skipped(test, "this test should be skipped"); > + > + /* This line should run */ > + kunit_log(KERN_INFO, test, "You should see this line."); > +} > + > +static struct kunit_case example_skip_test_cases[] = { > + KUNIT_CASE(example_skip_test), > + KUNIT_CASE(example_mark_skipped_test), > + {} > +}; > + > +static struct kunit_suite example_skip_test_suite = { > + .name = "example_skip", > + .test_cases = example_skip_test_cases, > +}; > + > +kunit_test_suites(&example_skip_test_suite); > + > +MODULE_LICENSE("GPL v2"); > -- > 2.31.1.818.g46aad6cb9e-goog > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210526081112.3652290-3-davidgow%40google.com.
On Thu, May 27, 2021 at 2:29 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Wed, May 26, 2021 at 1:56 AM 'Marco Elver' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > On Wed, May 26, 2021 at 01:11AM -0700, David Gow wrote: > > > Add a new KUnit test suite which contains tests which are always > > > skipped. This is used as an example for how to write tests which are > > > skipped, and to demonstrate the difference between kunit_skip() and > > > kunit_mark_skipped(). > > > > > > Because these tests do not pass (they're skipped), they are not enabled > > > by default, or by the KUNIT_ALL_TESTS config option: they must be > > > enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either > > > a .config or .kunitconfig file. > > > > > > Signed-off-by: David Gow <davidgow@google.com> > > > --- > > > lib/kunit/Kconfig | 15 +++++++++ > > > lib/kunit/Makefile | 2 ++ > > > lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ > > > 3 files changed, 69 insertions(+) > > > create mode 100644 lib/kunit/kunit-example-skip-test.c > > > > I don't know if this test is useful for a user of KUnit. Given it's not > > testing KUnit functionality (I see you added tests that the feature > > works in patch 1/3), but rather a demonstration and therefore dead code. > > I don't think the feature is difficult to understand from the API doc > > text. > > > > Instead, would it be more helpful to add something to > > Documentation/dev-tools/kunit? Or perhaps just add something to > > lib/kunit/kunit-example-test.c? It'd avoid introducing more Kconfig > > I'm in favor of putting it in kunit-example-test.c as well. > > But I hear there was pushback to have a non-passing test in the example? > I guess the fear is that someone will see something that doesn't say > "passed" in the example output and think something has gone wrong? Yeah, (a simpler version of) this was in kunit-example-test.c before. Brendan brought up the question of if a test which skips all the time is useful in [1], but that was more in the context of this being a test of the functionality than an example. The other part of this did grow out of the discussion for whether skipped tests should be treated as 'ok' or 'not ok' in the KTAP spec (somewhere in [2], probably), and whether or not a test being skipped was indicative of something going wrong. Ultimately, I think there's some value in having an example test that's skipped, and if people are okay with it being in the example suite (and therefore enabled by default), I'm okay with the default KUnit run having a splotch of yellow amongst the green in the kunit_tool output. > Hence this more conservative change. > But I hope that in the absence of any replies in opposition, we can > just keep one example-test.c > Agreed, I'll put this back in the existing example suite for v2: if there's any great opposition, I can always move it back. > > options at least. > > > > Thanks, > > -- Marco > > Cheers, -- David [1]: https://lore.kernel.org/linux-kselftest/CAFd5g47auKoQPhCeMHSTMtE_9+fZ6eOHZkojV5j0AX4N4hE_pw@mail.gmail.com/ [2]: https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@CY4PR13MB1175.namprd13.prod.outlook.com/
diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index 0b5dfb001bac..399fe5f789f7 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -45,6 +45,21 @@ config KUNIT_EXAMPLE_TEST is intended for curious hackers who would like to understand how to use KUnit for kernel development. +config KUNIT_EXAMPLE_SKIP_TEST + tristate "Skipped test example for KUnit" + default n + help + Enables an example unit test that is always skipped. + + This test only exists to help new users understand what KUnit is and + how it is used. Please refer to the example test itself, + lib/kunit/example-test.c, for more information. This option is + intended for curious hackers who would like to understand how to use + KUnit for kernel development. + + Because this test does not pass, it is not enabled by + CONFIG_KUNIT_ALL_TESTS + config KUNIT_ALL_TESTS tristate "All KUnit tests with satisfied dependencies" help diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index c49f4ffb6273..8a99ff2f83bd 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -18,3 +18,5 @@ obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o endif obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o + +obj-$(CONFIG_KUNIT_EXAMPLE_SKIP_TEST) += kunit-example-skip-test.o diff --git a/lib/kunit/kunit-example-skip-test.c b/lib/kunit/kunit-example-skip-test.c new file mode 100644 index 000000000000..5395ee0be485 --- /dev/null +++ b/lib/kunit/kunit-example-skip-test.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Example KUnit test which is always skipped. + * + * Copyright (C) 2021, Google LLC. + * Author: David Gow <davidgow@google.com> + */ + +#include <kunit/test.h> + +/* + * This test should always be skipped. + */ + +static void example_skip_test(struct kunit *test) +{ + /* This line should run */ + kunit_log(KERN_INFO, test, "You should not see a line below."); + + /* Skip (and abort) the test */ + kunit_skip(test, "this test should be skipped"); + + /* This line should not execute */ + kunit_log(KERN_INFO, test, "You should not see this line."); +} + +static void example_mark_skipped_test(struct kunit *test) +{ + /* This line should run */ + kunit_log(KERN_INFO, test, "You should see a line below."); + + /* Skip (but do not abort) the test */ + kunit_mark_skipped(test, "this test should be skipped"); + + /* This line should run */ + kunit_log(KERN_INFO, test, "You should see this line."); +} + +static struct kunit_case example_skip_test_cases[] = { + KUNIT_CASE(example_skip_test), + KUNIT_CASE(example_mark_skipped_test), + {} +}; + +static struct kunit_suite example_skip_test_suite = { + .name = "example_skip", + .test_cases = example_skip_test_cases, +}; + +kunit_test_suites(&example_skip_test_suite); + +MODULE_LICENSE("GPL v2");
Add a new KUnit test suite which contains tests which are always skipped. This is used as an example for how to write tests which are skipped, and to demonstrate the difference between kunit_skip() and kunit_mark_skipped(). Because these tests do not pass (they're skipped), they are not enabled by default, or by the KUNIT_ALL_TESTS config option: they must be enabled explicitly by setting CONFIG_KUNIT_EXAMPLE_SKIP_TEST=y in either a .config or .kunitconfig file. Signed-off-by: David Gow <davidgow@google.com> --- lib/kunit/Kconfig | 15 +++++++++ lib/kunit/Makefile | 2 ++ lib/kunit/kunit-example-skip-test.c | 52 +++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) create mode 100644 lib/kunit/kunit-example-skip-test.c