Re: [PATCH 3/3] kunit: test: Add example_skip test suite which is always skipped

From: Marco Elver <elver@google.com>
To: Daniel Latypov <dlatypov@google.com>
Cc: David Gow <davidgow@google.com>,
	Brendan Higgins <brendanhiggins@google.com>,
	Alan Maguire <alan.maguire@oracle.com>,
	Shuah Khan <skhan@linuxfoundation.org>,
	KUnit Development <kunit-dev@googlegroups.com>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] kunit: test: Add example_skip test suite which is always skipped
Date: Wed, 26 May 2021 20:35:47 +0200	[thread overview]
Message-ID: <YK6VA8mHSg4aU1Ts@elver.google.com> (raw)
In-Reply-To: <CAGS_qxp=EV1iy5tCs+YpxH-Pug=MDTBXo3jSc13-h7HJnzBnDA@mail.gmail.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