CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook
[off-list ref] wrote:

On Sat, Jun 13, 2020 at 02:51:17PM +0800, David Gow wrote:

quoted

Yeah, _KUNIT_TEST was what we've sort-of implicitly decided on for
config names, but the documentation does need to happen.

That works for me. It still feels redundant, but all I really want is a
standard name. :)

quoted

We haven't put as much thought into standardising the filenames much, though.

I actually find this to be much more important because it is more
end-user-facing (i.e. in module naming, in build logs, in scripts, on
filesystem, etc -- CONFIG is basically only present during kernel build).
Trying to do any sorting or greping really needs a way to find all the
kunit pieces.

Certainly this is more of an issue now we support building KUnit tests
as modules, rather than having them always be built-in.

Having some halfway consistent config-name <-> filename <-> test suite
name could be useful down the line, too. Unfortunately, not
necessarily a 1:1 mapping, e.g.:
- CONFIG_KUNIT_TEST compiles both kunit-test.c and string-stream-test.c
- kunit-test.c has several test suites within it:
kunit-try-catch-test, kunit-resource-test & kunit-log-test.
- CONFIG_EXT4_KUNIT_TESTS currently only builds ext4-inode-test.c, but
as the plural name suggests, might build others later.
- CONFIG_SECURITY_APPARMOR_KUNIT_TEST doesn't actually have its own
source file: the test is built into policy_unpack.c
- &cetera

Indeed, this made me quickly look up the names of suites, and there
are a few inconsistencies there:
- most have "-test" as a suffix
- some have "_test" as a suffix
- some have no suffix

(I'm inclined to say that these don't need a suffix at all.)

Within test suites, we're also largely prefixing all of the tests with
a suite name (even if it's not actually the specified suite name). For
example, CONFIG_PM_QOS_KUNIT_TEST builds
drivers/base/power/qos-test.c which contains a suite called
"qos-kunit-test", with tests prefixed "freq_qos_test_". Some of this
clearly comes down to wanting to namespace things a bit more
("qos-test" as a name could refer to a few things, I imagine), but
specifying how to do so consistently could help.

quoted

Both of these are slightly complicated by cases like this where tests
are being ported from a non-KUnit test to KUnit. There's a small
argument there for trying to keep the name the same, though personally
I suspect consistency is more important.

Understood. I think consistency is preferred too, especially since the
driving reason to make this conversions is to gain consistency with the
actual tests themselves.

We'll go with that until someone objects: from what I recall, this is
largely what people have been doing anyway.

quoted

Alas, the plans to document test coding style / conventions kept
getting pre-empted: I'll drag it back up to the top of the to-do list,
and see if we can't prioritise it. I think we'd hoped to be able to
catch these in review, but between a bit of forgetfulness and a few
tests going upstream without our seeing them has made it obvious that
doesn't work.

Once something's documented (and the suitable bikeshedding has
subsided), we can consider renaming existing tests if that seems
worthwhile.

Yes please! :)

I'll see if I can find time to draft something this week, then. No
promises, but hopefully there'll at least be something to build on.

quoted

My feeling is we'll go for:
- Kconfig name: ~_KUNIT_TEST

As mentioned, I'm fine with this, but prefer ~_KUNIT

quoted

- filename: ~-test.c

I really don't like this. Several reasons reasons:

- it does not distinguish it from other tests -- there is no way to
  identify kunit tests from non-kunit tests from directory listings,
  build log greps, etc.

- the current "common" naming has been with a leading "test", ignoring
  kunit, tools/, and samples/:

        53 test_*.c
        27 *_test.c
        19 *[a-z0-9]test.c
        19 selftest*.c
        16 test-*.c
        11 *-test.c
        11 test[a-z0-9]*.c
         8 *-tests.c
         5 *-selftest.c
         4 *_test_*.c
         1 *_selftest_*.c
         1 *_selftests.c

(these counts might be a bit off -- my eyes started to cross while
constructing regexes)

- dashes are converted to _ in module names, leading to some confusion
  between .c file and .ko file.

I'd strongly prefer ~_kunit.c, but could live with _kunit_test.c (even
though it's redundant).

I suggested -test.c largely because it's seemed to be most popular out
of existing KUnit tests (and certainly out of the ones that already
had-KUNIT_TEST config suffixes), but I definitely see your points.
I think that trying to stick to a "common" test naming is a bit
contradictory with trying to distinguish KUnit tests from other tests,
and I'm leaning to the side of distinguishing them, so I definitely
could be converted to ~_kunit.c.

Brendan, any thoughts?

quoted

At least for the initial draft documentation, as those seem to be most
common, but I think a thread on that would probably be the best place
to add it.

I'm ready! :) (Subject updated)

quoted

This would also be a good opportunity to document the "standard" KUnit
boilerplate help text in the Kconfig options.

Ah yeah, good idea.

--
Kees Cook

Cheers,
-- David
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees