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

[David Gow]

On Thu, May 27, 2021 at 2:29 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Wed, May 26, 2021 at 1:56 AM 'Marco Elver' via KUnit Development
> <kunit-dev@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature