[RFC,v3,01/19] kunit: test: add KUnit test runner core

Message ID 20181128193636.254378-2-brendanhiggins@google.com
State Needs Review / ACK
Headers show
Series
  • kunit: introduce KUnit, the Linux kernel unit testing framework
Related show

Commit Message

Brendan Higgins Nov. 28, 2018, 7:36 p.m.
Add core facilities for defining unit tests; this provides a common way
to define test cases, functions that execute code which is under test
and determine whether the code under test behaves as expected; this also
provides a way to group together related test cases in test suites (here
we call them test_modules).

Just define test cases and how to execute them for now; setting
expectations on code will be defined later.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 include/kunit/test.h | 165 ++++++++++++++++++++++++++++++++++++++++++
 kunit/Kconfig        |  17 +++++
 kunit/Makefile       |   1 +
 kunit/test.c         | 168 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 351 insertions(+)
 create mode 100644 include/kunit/test.h
 create mode 100644 kunit/Kconfig
 create mode 100644 kunit/Makefile
 create mode 100644 kunit/test.c

Comments

Luis Chamberlain Nov. 30, 2018, 3:14 a.m. | #1
On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> +#define module_test(module) \
> +		static int module_kunit_init##module(void) \
> +		{ \
> +			return kunit_run_tests(&module); \
> +		} \
> +		late_initcall(module_kunit_init##module)

Here in lies an assumption that suffices. I'm inclined to believe we
need new initcall level here so to ensure we *do* run after all the
respective kernels iniut calls. Otherwise we're left at the whims of
link order for kunit. For instance if a kunit test relies on frameworks
which are also late_initcall() we'd have complete incompatibility with
anything linked *after* kunit.

> diff --git a/kunit/Kconfig b/kunit/Kconfig
> new file mode 100644
> index 0000000000000..49b44c4f6630a
> --- /dev/null
> +++ b/kunit/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# KUnit base configuration
> +#
> +
> +menu "KUnit support"
> +
> +config KUNIT
> +	bool "Enable support for unit tests (KUnit)"
> +	depends on UML

Consider using:

if UML
   ...
endif

That allows the depends to be done once.

> +	help
> +	  Enables support for kernel unit tests (KUnit), a lightweight unit
> +	  testing and mocking framework for the Linux kernel. These tests are
> +	  able to be run locally on a developer's workstation without a VM or
> +	  special hardware.


Some mention of UML may be good here?

> For more information, please see
> +	  Documentation/kunit/
> +
> +endmenu

I'm a bit conflicted here. This currently depends on UML but yet you
noted on RFC v2 that your intention is to liberate kunit from UML and
ideally allow unit tests to depend only on userspace. I've addressed
tests using both selftests kernels drivers and also re-written kernel
APIs to userspace to test there. I think we may need to live with both.

Then for the UML stuff, I think if we *really* accept that UML will
always be a viable option we should probably consider now throwing these
things under drivers/platform/uml/. This follows the pattern of arch
specific drivers. Whether or not we end up with a complete userspace
component independent of UML may implicate having a shared component
somewhere else.

Likewise, I realize the goal is to *avoid* using a virtual machine for
these tests, but would it in any way make sense to share kunit to be
supported for other architectures to allow easier-to-write tests as
well?

  Luis
Luis Chamberlain Nov. 30, 2018, 3:28 a.m. | #2
> +static void kunit_run_case_internal(struct kunit *test,
> +				    struct kunit_module *module,
> +				    struct kunit_case *test_case)
> +{
> +	int ret;
> +
> +	if (module->init) {
> +		ret = module->init(test);
> +		if (ret) {
> +			kunit_err(test, "failed to initialize: %d", ret);
> +			kunit_set_success(test, false);
> +			return;
> +		}
> +	}
> +
> +	test_case->run_case(test);
> +}

<-- snip -->

> +static bool kunit_run_case(struct kunit *test,
> +			   struct kunit_module *module,
> +			   struct kunit_case *test_case)
> +{
> +	kunit_set_success(test, true);
> +
> +	kunit_run_case_internal(test, module, test_case);
> +	kunit_run_case_cleanup(test, module, test_case);
> +
> +	return kunit_get_success(test);
> +}

So we are running the module->init() for each test case... is that
correct? Shouldn't the init run once? Also, typically init calls are
pegged with __init so we free them later. You seem to have skipped the
init annotations. Why?

  Luis
Brendan Higgins Dec. 1, 2018, 1:51 a.m. | #3
On Thu, Nov 29, 2018 at 7:14 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> > +#define module_test(module) \
> > +             static int module_kunit_init##module(void) \
> > +             { \
> > +                     return kunit_run_tests(&module); \
> > +             } \
> > +             late_initcall(module_kunit_init##module)
>
> Here in lies an assumption that suffices. I'm inclined to believe we
> need new initcall level here so to ensure we *do* run after all the
> respective kernels iniut calls. Otherwise we're left at the whims of
> link order for kunit. For instance if a kunit test relies on frameworks
> which are also late_initcall() we'd have complete incompatibility with
> anything linked *after* kunit.

Yep, I have some patches that address this, but I thought this is
sufficient for the initial patchset (I figured that's the type of
thing that people will have opinions about so best to get it out of
the critical path). Do you want me to add those in the next revision?

>
> > diff --git a/kunit/Kconfig b/kunit/Kconfig
> > new file mode 100644
> > index 0000000000000..49b44c4f6630a
> > --- /dev/null
> > +++ b/kunit/Kconfig
> > @@ -0,0 +1,17 @@
> > +#
> > +# KUnit base configuration
> > +#
> > +
> > +menu "KUnit support"
> > +
> > +config KUNIT
> > +     bool "Enable support for unit tests (KUnit)"
> > +     depends on UML
>
> Consider using:
>
> if UML
>    ...
> endif
>
> That allows the depends to be done once.

If you want to eliminate depends, wouldn't it be best to have KUNIT
depend on whatever it needs, and then do `if KUNIT` below that? That
seems cleaner over the long term. Anyway, Kees actually asked me to
change it to the way it is now; I really don't care either way.

>
> > +     help
> > +       Enables support for kernel unit tests (KUnit), a lightweight unit
> > +       testing and mocking framework for the Linux kernel. These tests are
> > +       able to be run locally on a developer's workstation without a VM or
> > +       special hardware.
>
>
> Some mention of UML may be good here?

Good point.
>
> > For more information, please see
> > +       Documentation/kunit/
> > +
> > +endmenu
>
> I'm a bit conflicted here. This currently depends on UML but yet you
> noted on RFC v2 that your intention is to liberate kunit from UML and
> ideally allow unit tests to depend only on userspace. I've addressed
> tests using both selftests kernels drivers and also re-written kernel
> APIs to userspace to test there. I think we may need to live with both.

I am not entirely opposed. The greater isolation we can achieve, the
fewer dependencies, and barriers to setting up test fixtures the
better. I think the best way to do that in most cases is allowing
minimal test binaries to be built that have the absolute minimum
amount of code necessary to test the desired property. That being
said, integration tests are a thing and drawing a line between them
and unit tests is not always possible, so supporting other
architectures might be necessary.

>
> Then for the UML stuff, I think if we *really* accept that UML will
> always be a viable option we should probably consider now throwing these
> things under drivers/platform/uml/. This follows the pattern of arch
> specific drivers. Whether or not we end up with a complete userspace
> component independent of UML may implicate having a shared component
> somewhere else.

Fair enough. What specifically are you suggesting should go in
`drivers/platform/uml`? Just the bits that are completely tied to UML
or a concrete architecture?

>
> Likewise, I realize the goal is to *avoid* using a virtual machine for
> these tests, but would it in any way make sense to share kunit to be
> supported for other architectures to allow easier-to-write tests as
> well?

You are not the first person to ask for this.

For the vast majority of tests, I think we can (and consequently
should) make them run without any external dependencies. Doing so
makes it such that someone can run a test without knowing anything
about it, which allows you to do a lot of things. For one, I, as a
developer, don't have to hunt down somebody's QEMU patches, or
whatever. But it also means I, as someone maintaining part of the
kernel, can make nice test runners and build things like presubmit
servers on top of them.

Nevertheless, I accept that there are things which are just easier to
do with hardware or a VM (for integration tests it is necessary).
Still, I think we need to make sure the vast majority of unit tests do
not depend on real hardware or a VM.
Brendan Higgins Dec. 1, 2018, 2:08 a.m. | #4
On Thu, Nov 29, 2018 at 7:28 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> > +static void kunit_run_case_internal(struct kunit *test,
> > +                                 struct kunit_module *module,
> > +                                 struct kunit_case *test_case)
> > +{
> > +     int ret;
> > +
> > +     if (module->init) {
> > +             ret = module->init(test);
> > +             if (ret) {
> > +                     kunit_err(test, "failed to initialize: %d", ret);
> > +                     kunit_set_success(test, false);
> > +                     return;
> > +             }
> > +     }
> > +
> > +     test_case->run_case(test);
> > +}
>
> <-- snip -->
>
> > +static bool kunit_run_case(struct kunit *test,
> > +                        struct kunit_module *module,
> > +                        struct kunit_case *test_case)
> > +{
> > +     kunit_set_success(test, true);
> > +
> > +     kunit_run_case_internal(test, module, test_case);
> > +     kunit_run_case_cleanup(test, module, test_case);
> > +
> > +     return kunit_get_success(test);
> > +}
>
> So we are running the module->init() for each test case... is that
> correct? Shouldn't the init run once? Also, typically init calls are

Yep, it's correct. `module->init()` should run once before every test
case, reason being that the kunit_module serves as a test fixture in
which each test cases should be run completely independently of every
other. init and exit is supposed to allow code common to all test
cases to run since it is so common to have dependencies needed for a
test to be common to every test case.

Maybe it is confusing that I call it kunit_module? Maybe I should call
it kunit_fixture or something?

> pegged with __init so we free them later. You seem to have skipped the
> init annotations. Why?

Like I said above, these aren't normal init functions. A
kunit_module->init() function should run once before each test case
and thus should reside in the same linker section as any other KUnit
test code.

Cheers
Luis Chamberlain Dec. 1, 2018, 2:57 a.m. | #5
On Fri, Nov 30, 2018 at 05:51:11PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:14 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> > > +#define module_test(module) \
> > > +             static int module_kunit_init##module(void) \
> > > +             { \
> > > +                     return kunit_run_tests(&module); \
> > > +             } \
> > > +             late_initcall(module_kunit_init##module)
> >
> > Here in lies an assumption that suffices. I'm inclined to believe we
> > need new initcall level here so to ensure we *do* run after all the
> > respective kernels iniut calls. Otherwise we're left at the whims of
> > link order for kunit. For instance if a kunit test relies on frameworks
> > which are also late_initcall() we'd have complete incompatibility with
> > anything linked *after* kunit.
> 
> Yep, I have some patches that address this, but I thought this is
> sufficient for the initial patchset (I figured that's the type of
> thing that people will have opinions about so best to get it out of
> the critical path). Do you want me to add those in the next revision?
> 
> >
> > > diff --git a/kunit/Kconfig b/kunit/Kconfig
> > > new file mode 100644
> > > index 0000000000000..49b44c4f6630a
> > > --- /dev/null
> > > +++ b/kunit/Kconfig
> > > @@ -0,0 +1,17 @@
> > > +#
> > > +# KUnit base configuration
> > > +#
> > > +
> > > +menu "KUnit support"
> > > +
> > > +config KUNIT
> > > +     bool "Enable support for unit tests (KUnit)"
> > > +     depends on UML
> >
> > Consider using:
> >
> > if UML
> >    ...
> > endif
> >
> > That allows the depends to be done once.
> 
> If you want to eliminate depends, wouldn't it be best to have KUNIT
> depend on whatever it needs, and then do `if KUNIT` below that? That
> seems cleaner over the long term. Anyway, Kees actually asked me to
> change it to the way it is now; I really don't care either way.

Yes, that works better. The idea is to just avoid having to write in
depends on over and over again.

> > I'm a bit conflicted here. This currently depends on UML but yet you
> > noted on RFC v2 that your intention is to liberate kunit from UML and
> > ideally allow unit tests to depend only on userspace. I've addressed
> > tests using both selftests kernels drivers and also re-written kernel
> > APIs to userspace to test there. I think we may need to live with both.
> 
> I am not entirely opposed. The greater isolation we can achieve, the
> fewer dependencies, and barriers to setting up test fixtures the
> better. I think the best way to do that in most cases is allowing
> minimal test binaries to be built that have the absolute minimum
> amount of code necessary to test the desired property. That being
> said, integration tests are a thing and drawing a line between them
> and unit tests is not always possible, so supporting other
> architectures might be necessary.

Then lets pave the way for it to be done easily.

> > Then for the UML stuff, I think if we *really* accept that UML will
> > always be a viable option we should probably consider now throwing these
> > things under drivers/platform/uml/. This follows the pattern of arch
> > specific drivers. Whether or not we end up with a complete userspace
> > component independent of UML may implicate having a shared component
> > somewhere else.
> 
> Fair enough. What specifically are you suggesting should go in
> `drivers/platform/uml`? Just the bits that are completely tied to UML
> or a concrete architecture?

The bits that are UML specific. As I see it, with the above intention
clarified, kunit is a framework for architectures and UML is supported
first. The code doesn't currently reflect this.

> > Likewise, I realize the goal is to *avoid* using a virtual machine for
> > these tests, but would it in any way make sense to share kunit to be
> > supported for other architectures to allow easier-to-write tests as
> > well?
> 
> You are not the first person to ask for this.
> 
> For the vast majority of tests, I think we can (and consequently
> should) make them run without any external dependencies. Doing so
> makes it such that someone can run a test without knowing anything
> about it, which allows you to do a lot of things. For one, I, as a
> developer, don't have to hunt down somebody's QEMU patches, or
> whatever. But it also means I, as someone maintaining part of the
> kernel, can make nice test runners and build things like presubmit
> servers on top of them.
> 
> Nevertheless, I accept that there are things which are just easier to
> do with hardware or a VM (for integration tests it is necessary).
> Still, I think we need to make sure the vast majority of unit tests do
> not depend on real hardware or a VM.

When possible, sure.

  Luis
Luis Chamberlain Dec. 1, 2018, 3:02 a.m. | #6
On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> +int kunit_run_tests(struct kunit_module *module)
> +{
> +	bool all_passed = true, success;
> +	struct kunit_case *test_case;
> +	struct kunit test;
> +	int ret;
> +
> +	ret = kunit_init_test(&test, module->name);
> +	if (ret)
> +		return ret;
> +
> +	for (test_case = module->test_cases; test_case->run_case; test_case++) {
> +		success = kunit_run_case(&test, module, test_case);

We are running test cases serially, why not address testing
asynchronously, this way tests can also be paralellized when possible,
therefore decreasing test time even further.

Would that mess up the printing/log stuff somehow?

  Luis
Luis Chamberlain Dec. 1, 2018, 3:10 a.m. | #7
On Fri, Nov 30, 2018 at 06:08:36PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:28 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > > +static void kunit_run_case_internal(struct kunit *test,
> > > +                                 struct kunit_module *module,
> > > +                                 struct kunit_case *test_case)
> > > +{
> > > +     int ret;
> > > +
> > > +     if (module->init) {
> > > +             ret = module->init(test);
> > > +             if (ret) {
> > > +                     kunit_err(test, "failed to initialize: %d", ret);
> > > +                     kunit_set_success(test, false);
> > > +                     return;
> > > +             }
> > > +     }
> > > +
> > > +     test_case->run_case(test);
> > > +}
> >
> > <-- snip -->
> >
> > > +static bool kunit_run_case(struct kunit *test,
> > > +                        struct kunit_module *module,
> > > +                        struct kunit_case *test_case)
> > > +{
> > > +     kunit_set_success(test, true);
> > > +
> > > +     kunit_run_case_internal(test, module, test_case);
> > > +     kunit_run_case_cleanup(test, module, test_case);
> > > +
> > > +     return kunit_get_success(test);
> > > +}
> >
> > So we are running the module->init() for each test case... is that
> > correct? Shouldn't the init run once? Also, typically init calls are
> 
> Yep, it's correct. `module->init()` should run once before every test
> case, reason being that the kunit_module serves as a test fixture in
> which each test cases should be run completely independently of every
> other.

Shouldn't the init be test_case specific as well? Right now we just
past the struct kunit, but not the struct kunit_case. I though that
that the struct kunit_case was where we'd customize each specific
test case as we see fit for each test case. If not, how would we
do say, a different type of initialization for a different type of
test (for the same unit)?

> init and exit is supposed to allow code common to all test
> cases to run since it is so common to have dependencies needed for a
> test to be common to every test case.

Sure things in common make sense, however the differntiating aspects
seem important as well on init? Or should the author be doing all
custom specific initializations on run_case() instead?

  Luis
Brendan Higgins Dec. 3, 2018, 10:47 p.m. | #8
On Fri, Nov 30, 2018 at 7:10 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, Nov 30, 2018 at 06:08:36PM -0800, Brendan Higgins wrote:
> > On Thu, Nov 29, 2018 at 7:28 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > > +static void kunit_run_case_internal(struct kunit *test,
> > > > +                                 struct kunit_module *module,
> > > > +                                 struct kunit_case *test_case)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     if (module->init) {
> > > > +             ret = module->init(test);
> > > > +             if (ret) {
> > > > +                     kunit_err(test, "failed to initialize: %d", ret);
> > > > +                     kunit_set_success(test, false);
> > > > +                     return;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     test_case->run_case(test);
> > > > +}
> > >
> > > <-- snip -->
> > >
> > > > +static bool kunit_run_case(struct kunit *test,
> > > > +                        struct kunit_module *module,
> > > > +                        struct kunit_case *test_case)
> > > > +{
> > > > +     kunit_set_success(test, true);
> > > > +
> > > > +     kunit_run_case_internal(test, module, test_case);
> > > > +     kunit_run_case_cleanup(test, module, test_case);
> > > > +
> > > > +     return kunit_get_success(test);
> > > > +}
> > >
> > > So we are running the module->init() for each test case... is that
> > > correct? Shouldn't the init run once? Also, typically init calls are
> >
> > Yep, it's correct. `module->init()` should run once before every test
> > case, reason being that the kunit_module serves as a test fixture in
> > which each test cases should be run completely independently of every
> > other.
>
> Shouldn't the init be test_case specific as well? Right now we just
> past the struct kunit, but not the struct kunit_case. I though that
> that the struct kunit_case was where we'd customize each specific
> test case as we see fit for each test case. If not, how would we
> do say, a different type of initialization for a different type of
> test (for the same unit)?

Maybe there should be other init functions, but specifying an init
function per case is not typical. In most unit testing frameworks
there is some sort of optional per test case init function that sets
up the fixture common to all cases; it is also fairly common to have
an init function that runs once at the very beginning of the entire
test suite (like what you thought I was doing); however, it is not
used nearly as often as the former, and even then is usually used in
conjunction with the former.

Nevertheless, I don't think I have ever seen a unit test framework
provide a way to make init functions specific to each case. I don't
see any good reason not to do it other than the lack of examples in
the wild suggest it would not get much usage.

In general, some limited initialization specific to a test case is
allowed in the test case itself, and if you have really complicated
initialization that warrants a separate init function, but isn't
shared between cases, you should probably put the test in a separate
test suite with a separate test fixture. I am sure there will be edge
cases that don't fit, but there is no technical reason why you cannot
just do the initialization in the test case itself in these cases.

>
> > init and exit is supposed to allow code common to all test
> > cases to run since it is so common to have dependencies needed for a
> > test to be common to every test case.
>
> Sure things in common make sense, however the differntiating aspects
> seem important as well on init? Or should the author be doing all
> custom specific initializations on run_case() instead?
>

Usually limited initialization specific to a test case will just go in
that test case.

Cheers
Anton Ivanov Dec. 5, 2018, 1:15 p.m. | #9
On 30/11/2018 03:14, Luis Chamberlain wrote:
> On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
>> +#define module_test(module) \
>> +		static int module_kunit_init##module(void) \
>> +		{ \
>> +			return kunit_run_tests(&module); \
>> +		} \
>> +		late_initcall(module_kunit_init##module)
> Here in lies an assumption that suffices. I'm inclined to believe we
> need new initcall level here so to ensure we *do* run after all the
> respective kernels iniut calls. Otherwise we're left at the whims of
> link order for kunit. For instance if a kunit test relies on frameworks
> which are also late_initcall() we'd have complete incompatibility with
> anything linked *after* kunit.
>
>> diff --git a/kunit/Kconfig b/kunit/Kconfig
>> new file mode 100644
>> index 0000000000000..49b44c4f6630a
>> --- /dev/null
>> +++ b/kunit/Kconfig
>> @@ -0,0 +1,17 @@
>> +#
>> +# KUnit base configuration
>> +#
>> +
>> +menu "KUnit support"
>> +
>> +config KUNIT
>> +	bool "Enable support for unit tests (KUnit)"
>> +	depends on UML
> Consider using:
>
> if UML
>     ...
> endif
>
> That allows the depends to be done once.
>
>> +	help
>> +	  Enables support for kernel unit tests (KUnit), a lightweight unit
>> +	  testing and mocking framework for the Linux kernel. These tests are
>> +	  able to be run locally on a developer's workstation without a VM or
>> +	  special hardware.
>
> Some mention of UML may be good here?
>
>> For more information, please see
>> +	  Documentation/kunit/
>> +
>> +endmenu
> I'm a bit conflicted here. This currently depends on UML but yet you
> noted on RFC v2 that your intention is to liberate kunit from UML and
> ideally allow unit tests to depend only on userspace. I've addressed
> tests using both selftests kernels drivers and also re-written kernel
> APIs to userspace to test there. I think we may need to live with both.
>
> Then for the UML stuff, I think if we *really* accept that UML will
> always be a viable option we should probably consider now throwing these
> things under drivers/platform/uml/. This follows the pattern of arch
> specific drivers. Whether or not we end up with a complete userspace

UML platform drivers predate that and are under arch/um/drivers/

We should either keep to current convention or consider relocating the 
existing ones - having things spread in different places around the tree 
is not good in the long run (UML already has a few of those under the 
x86 tree, let's not increase the number).

> component independent of UML may implicate having a shared component
> somewhere else.
>
> Likewise, I realize the goal is to *avoid* using a virtual machine for
> these tests, but would it in any way make sense to share kunit to be
> supported for other architectures to allow easier-to-write tests as
> well?
>
>    Luis
>
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
>
Arnd Bergmann Dec. 5, 2018, 2:45 p.m. | #10
On Wed, Dec 5, 2018 at 2:42 PM Anton Ivanov
<anton.ivanov@cambridgegreys.com> wrote:
> On 30/11/2018 03:14, Luis Chamberlain wrote:
> > On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
> > Then for the UML stuff, I think if we *really* accept that UML will
> > always be a viable option we should probably consider now throwing these
> > things under drivers/platform/uml/. This follows the pattern of arch
> > specific drivers. Whether or not we end up with a complete userspace
>
> UML platform drivers predate that and are under arch/um/drivers/
>
> We should either keep to current convention or consider relocating the
> existing ones - having things spread in different places around the tree
> is not good in the long run (UML already has a few of those under the
> x86 tree, let's not increase the number).

I don't mind the current location much, but if we move drivers, we should
move the into the appropriate subsystems based on what they do, rather
than having a new place with a mix of things.

E.g. the tty drivers should all be in drivers/tty/ and the network drivers in
drivers/net. To paraphrase what you said above: having tty drivers spread in
different places around the tree is not good in the long run. We have long
ago moved from organizing drivers by bus interface to organizing drivers
by class, uml and drivers/platform are just exceptions to this rule.

          Arnd
Anton Ivanov Dec. 5, 2018, 2:49 p.m. | #11
On 05/12/2018 14:45, Arnd Bergmann wrote:
> On Wed, Dec 5, 2018 at 2:42 PM Anton Ivanov
> <anton.ivanov@cambridgegreys.com> wrote:
>> On 30/11/2018 03:14, Luis Chamberlain wrote:
>>> On Wed, Nov 28, 2018 at 11:36:18AM -0800, Brendan Higgins wrote:
>>> Then for the UML stuff, I think if we *really* accept that UML will
>>> always be a viable option we should probably consider now throwing these
>>> things under drivers/platform/uml/. This follows the pattern of arch
>>> specific drivers. Whether or not we end up with a complete userspace
>> UML platform drivers predate that and are under arch/um/drivers/
>>
>> We should either keep to current convention or consider relocating the
>> existing ones - having things spread in different places around the tree
>> is not good in the long run (UML already has a few of those under the
>> x86 tree, let's not increase the number).
> I don't mind the current location much, but if we move drivers, we should
> move the into the appropriate subsystems based on what they do, rather
> than having a new place with a mix of things.
>
> E.g. the tty drivers should all be in drivers/tty/ and the network drivers in
> drivers/net. To paraphrase what you said above: having tty drivers spread in
> different places around the tree is not good in the long run. We have long
> ago moved from organizing drivers by bus interface to organizing drivers
> by class, uml and drivers/platform are just exceptions to this rule.

There are some issues with that because uml drivers have bits of what is 
effectively host side of the hypervisor as a part of them. IMHO, having 
that in driver/X is not very appropriate. So at least the *_user.c and 
*_user.h bits have to go (or stay) somewhere else

Brgds,

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
new file mode 100644
index 0000000000000..ffe66bb355d63
--- /dev/null
+++ b/include/kunit/test.h
@@ -0,0 +1,165 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2018, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_TEST_H
+#define _KUNIT_TEST_H
+
+#include <linux/types.h>
+#include <linux/slab.h>
+
+struct kunit;
+
+/**
+ * struct kunit_case - represents an individual test case.
+ * @run_case: the function representing the actual test case.
+ * @name: the name of the test case.
+ *
+ * A test case is a function with the signature, ``void (*)(struct kunit *)``
+ * that makes expectations (see KUNIT_EXPECT_TRUE()) about code under test. Each
+ * test case is associated with a &struct kunit_module and will be run after the
+ * module's init function and followed by the module's exit function.
+ *
+ * A test case should be static and should only be created with the KUNIT_CASE()
+ * macro; additionally, every array of test cases should be terminated with an
+ * empty test case.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ *	void add_test_basic(struct kunit *test)
+ *	{
+ *		KUNIT_EXPECT_EQ(test, 1, add(1, 0));
+ *		KUNIT_EXPECT_EQ(test, 2, add(1, 1));
+ *		KUNIT_EXPECT_EQ(test, 0, add(-1, 1));
+ *		KUNIT_EXPECT_EQ(test, INT_MAX, add(0, INT_MAX));
+ *		KUNIT_EXPECT_EQ(test, -1, add(INT_MAX, INT_MIN));
+ *	}
+ *
+ *	static struct kunit_case example_test_cases[] = {
+ *		KUNIT_CASE(add_test_basic),
+ *		{},
+ *	};
+ *
+ */
+struct kunit_case {
+	void (*run_case)(struct kunit *test);
+	const char name[256];
+
+	/* private: internal use only. */
+	bool success;
+};
+
+/**
+ * KUNIT_CASE - A helper for creating a &struct kunit_case
+ * @test_name: a reference to a test case function.
+ *
+ * Takes a symbol for a function representing a test case and creates a
+ * &struct kunit_case object from it. See the documentation for
+ * &struct kunit_case for an example on how to use it.
+ */
+#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
+
+/**
+ * struct kunit_module - describes a related collection of &struct kunit_case s.
+ * @name: the name of the test. Purely informational.
+ * @init: called before every test case.
+ * @exit: called after every test case.
+ * @test_cases: a null terminated array of test cases.
+ *
+ * A kunit_module is a collection of related &struct kunit_case s, such that
+ * @init is called before every test case and @exit is called after every test
+ * case, similar to the notion of a *test fixture* or a *test class* in other
+ * unit testing frameworks like JUnit or Googletest.
+ *
+ * Every &struct kunit_case must be associated with a kunit_module for KUnit to
+ * run it.
+ */
+struct kunit_module {
+	const char name[256];
+	int (*init)(struct kunit *test);
+	void (*exit)(struct kunit *test);
+	struct kunit_case *test_cases;
+};
+
+/**
+ * struct kunit - represents a running instance of a test.
+ * @priv: for user to store arbitrary data. Commonly used to pass data created
+ * in the init function (see &struct kunit_module).
+ *
+ * Used to store information about the current context under which the test is
+ * running. Most of this data is private and should only be accessed indirectly
+ * via public functions; the one exception is @priv which can be used by the
+ * test writer to store arbitrary data.
+ */
+struct kunit {
+	void *priv;
+
+	/* private: internal use only. */
+	const char *name; /* Read only after initialization! */
+	spinlock_t lock; /* Gaurds all mutable test state. */
+	bool success; /* Protected by lock. */
+	void (*vprintk)(const struct kunit *test,
+			const char *level,
+			struct va_format *vaf);
+};
+
+int kunit_init_test(struct kunit *test, const char *name);
+
+int kunit_run_tests(struct kunit_module *module);
+
+/**
+ * module_test() - used to register a &struct kunit_module with KUnit.
+ * @module: a statically allocated &struct kunit_module.
+ *
+ * Registers @module with the test framework. See &struct kunit_module for more
+ * information.
+ */
+#define module_test(module) \
+		static int module_kunit_init##module(void) \
+		{ \
+			return kunit_run_tests(&module); \
+		} \
+		late_initcall(module_kunit_init##module)
+
+void __printf(3, 4) kunit_printk(const char *level,
+				 const struct kunit *test,
+				 const char *fmt, ...);
+
+/**
+ * kunit_info() - Prints an INFO level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints an info level message associated with the test module being run. Takes
+ * a variable number of format parameters just like printk().
+ */
+#define kunit_info(test, fmt, ...) \
+		kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_warn() - Prints a WARN level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * See kunit_info().
+ */
+#define kunit_warn(test, fmt, ...) \
+		kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_err() - Prints an ERROR level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * See kunit_info().
+ */
+#define kunit_err(test, fmt, ...) \
+		kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+
+#endif /* _KUNIT_TEST_H */
diff --git a/kunit/Kconfig b/kunit/Kconfig
new file mode 100644
index 0000000000000..49b44c4f6630a
--- /dev/null
+++ b/kunit/Kconfig
@@ -0,0 +1,17 @@ 
+#
+# KUnit base configuration
+#
+
+menu "KUnit support"
+
+config KUNIT
+	bool "Enable support for unit tests (KUnit)"
+	depends on UML
+	help
+	  Enables support for kernel unit tests (KUnit), a lightweight unit
+	  testing and mocking framework for the Linux kernel. These tests are
+	  able to be run locally on a developer's workstation without a VM or
+	  special hardware. For more information, please see
+	  Documentation/kunit/
+
+endmenu
diff --git a/kunit/Makefile b/kunit/Makefile
new file mode 100644
index 0000000000000..5efdc4dea2c08
--- /dev/null
+++ b/kunit/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_KUNIT) +=			test.o
diff --git a/kunit/test.c b/kunit/test.c
new file mode 100644
index 0000000000000..26d3d6d260e6c
--- /dev/null
+++ b/kunit/test.c
@@ -0,0 +1,168 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2018, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/sched.h>
+#include <linux/sched/debug.h>
+#include <os.h>
+#include <kunit/test.h>
+
+static bool kunit_get_success(struct kunit *test)
+{
+	unsigned long flags;
+	bool success;
+
+	spin_lock_irqsave(&test->lock, flags);
+	success = test->success;
+	spin_unlock_irqrestore(&test->lock, flags);
+
+	return success;
+}
+
+static void kunit_set_success(struct kunit *test, bool success)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&test->lock, flags);
+	test->success = success;
+	spin_unlock_irqrestore(&test->lock, flags);
+}
+
+static int kunit_vprintk_emit(const struct kunit *test,
+			      int level,
+			      const char *fmt,
+			      va_list args)
+{
+	return vprintk_emit(0, level, NULL, 0, fmt, args);
+}
+
+static int kunit_printk_emit(const struct kunit *test,
+			     int level,
+			     const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, fmt);
+	ret = kunit_vprintk_emit(test, level, fmt, args);
+	va_end(args);
+
+	return ret;
+}
+
+static void kunit_vprintk(const struct kunit *test,
+			  const char *level,
+			  struct va_format *vaf)
+{
+	kunit_printk_emit(test,
+			  level[1] - '0',
+			  "kunit %s: %pV", test->name, vaf);
+}
+
+int kunit_init_test(struct kunit *test, const char *name)
+{
+	spin_lock_init(&test->lock);
+	test->name = name;
+	test->vprintk = kunit_vprintk;
+
+	return 0;
+}
+
+/*
+ * Initializes and runs test case. Does not clean up or do post validations.
+ */
+static void kunit_run_case_internal(struct kunit *test,
+				    struct kunit_module *module,
+				    struct kunit_case *test_case)
+{
+	int ret;
+
+	if (module->init) {
+		ret = module->init(test);
+		if (ret) {
+			kunit_err(test, "failed to initialize: %d", ret);
+			kunit_set_success(test, false);
+			return;
+		}
+	}
+
+	test_case->run_case(test);
+}
+
+/*
+ * Performs post validations and cleanup after a test case was run.
+ * XXX: Should ONLY BE CALLED AFTER kunit_run_case_internal!
+ */
+static void kunit_run_case_cleanup(struct kunit *test,
+				   struct kunit_module *module,
+				   struct kunit_case *test_case)
+{
+	if (module->exit)
+		module->exit(test);
+}
+
+/*
+ * Performs all logic to run a test case.
+ */
+static bool kunit_run_case(struct kunit *test,
+			   struct kunit_module *module,
+			   struct kunit_case *test_case)
+{
+	kunit_set_success(test, true);
+
+	kunit_run_case_internal(test, module, test_case);
+	kunit_run_case_cleanup(test, module, test_case);
+
+	return kunit_get_success(test);
+}
+
+int kunit_run_tests(struct kunit_module *module)
+{
+	bool all_passed = true, success;
+	struct kunit_case *test_case;
+	struct kunit test;
+	int ret;
+
+	ret = kunit_init_test(&test, module->name);
+	if (ret)
+		return ret;
+
+	for (test_case = module->test_cases; test_case->run_case; test_case++) {
+		success = kunit_run_case(&test, module, test_case);
+		if (!success)
+			all_passed = false;
+
+		kunit_info(&test,
+			  "%s %s",
+			  test_case->name,
+			  success ? "passed" : "failed");
+	}
+
+	if (all_passed)
+		kunit_info(&test, "all tests passed");
+	else
+		kunit_info(&test, "one or more tests failed");
+
+	return 0;
+}
+
+void kunit_printk(const char *level,
+		  const struct kunit *test,
+		  const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	test->vprintk(test, level, &vaf);
+
+	va_end(args);
+}