[v5,00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
mbox series

Message ID 20190617082613.109131-1-brendanhiggins@google.com
Headers show
Series
  • kunit: introduce KUnit, the Linux kernel unit testing framework
Related show

Message

Brendan Higgins June 17, 2019, 8:25 a.m. UTC
## TL;DR

A not so quick follow-up to Stephen's suggestions on PATCH v4. Nothing
that really changes any functionality or usage with the minor exception
of a couple public functions that Stephen asked me to rename.
Nevertheless, a good deal of clean up and fixes. See changes below.

As for our current status, right now we got Reviewed-bys on all patches
except:

- [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
  list

However, it would probably be good to get reviews/acks from the
subsystem maintainers on:

- [PATCH v5 06/18] kbuild: enable building KUnit
- [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
  list
- [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit
- [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for
  sysctl.c:proc_dointvec()
- [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC
  SYSCTL section

Other than that, I think we should be good to go.

One last thing, I updated the background to include my thoughts on KUnit
vs. in kernel testing with kselftest in the background sections as
suggested by Frank in the discussion on PATCH v2.

## Background

This patch set proposes KUnit, a lightweight unit testing and mocking
framework for the Linux kernel.

Unlike Autotest and kselftest, KUnit is a true unit testing framework;
it does not require installing the kernel on a test machine or in a VM
(however, KUnit still allows you to run tests on test machines or in VMs
if you want[1]) and does not require tests to be written in userspace
running on a host kernel. Additionally, KUnit is fast: From invocation
to completion KUnit can run several dozen tests in under a second.
Currently, the entire KUnit test suite for KUnit runs in under a second
from the initial invocation (build time excluded).

KUnit is heavily inspired by JUnit, Python's unittest.mock, and
Googletest/Googlemock for C++. KUnit provides facilities for defining
unit test cases, grouping related test cases into test suites, providing
common infrastructure for running tests, mocking, spying, and much more.

### But wait! Doesn't kselftest support in kernel testing?!

In a previous version of this patchset Frank pointed out that kselftest
already supports writing a test that resides in the kernel using the
test module feature[2]. LWN did a really great summary on this
discussion here[3].

Kselftest has a feature that allows a test module to be loaded into a
kernel using the kselftest framework; this does allow someone to write
tests against kernel code not directly exposed to userland; however, it
does not provide much of a framework around how to structure the tests.
The kselftest test module feature just provides a header which has a
standardized way of reporting test failures, and then provides
infrastructure to load and run the tests using the kselftest test
harness.

The kselftest test module does not seem to be opinionated at all in
regards to how tests are structured, how they check for failures, how
tests are organized. Even in the method it provides for reporting
failures is pretty simple; it doesn't have any more advanced failure
reporting or logging features. Given what's there, I think it is fair to
say that it is not actually a framework, but a feature that makes it
possible for someone to do some checks in kernel space.

Furthermore, kselftest test module has very few users. I checked for all
the tests that use it using the following grep command:

grep -Hrn -e 'kselftest_module\.h'

and only got three results: lib/test_strscpy.c, lib/test_printf.c, and
lib/test_bitmap.c.

So despite kselftest test module's existence, there really is no feature
overlap between kselftest and KUnit, save one: that you can use either
to write an in-kernel test, but this is a very small feature in
comparison to everything that KUnit allows you to do. KUnit is a full
x-unit style unit testing framework, whereas kselftest looks a lot more
like an end-to-end/functional testing framework, with a feature that
makes it possible to write in-kernel tests.

### What's so special about unit testing?

A unit test is supposed to test a single unit of code in isolation,
hence the name. There should be no dependencies outside the control of
the test; this means no external dependencies, which makes tests orders
of magnitudes faster. Likewise, since there are no external dependencies,
there are no hoops to jump through to run the tests. Additionally, this
makes unit tests deterministic: a failing unit test always indicates a
problem. Finally, because unit tests necessarily have finer granularity,
they are able to test all code paths easily solving the classic problem
of difficulty in exercising error handling code.

### Is KUnit trying to replace other testing frameworks for the kernel?

No. Most existing tests for the Linux kernel are end-to-end tests, which
have their place. A well tested system has lots of unit tests, a
reasonable number of integration tests, and some end-to-end tests. KUnit
is just trying to address the unit test space which is currently not
being addressed.

### More information on KUnit

There is a bunch of documentation near the end of this patch set that
describes how to use KUnit and best practices for writing unit tests.
For convenience I am hosting the compiled docs here[4].

Additionally for convenience, I have applied these patches to a
branch[5]. The repo may be cloned with:
git clone https://kunit.googlesource.com/linux
This patchset is on the kunit/rfc/v5.2-rc4/v5 branch.

## Changes Since Last Version

Aside from a couple public function renames, there isn't really anything
in here that changes any functionality.

- Went through and fixed a couple of anti-patterns suggested by Stephen
  Boyd. Things like:
  - Dropping an else clause at the end of a function.
  - Dropping the comma on the closing sentinel, `{}`, of a list.
- Inlines a bunch of functions in the test case running logic in patch
  01/18 to make it more readable as suggested by Stephen Boyd
- Found and fixed bug in resource deallocation logic in patch 02/18. Bug
  was discovered as a result of making a change suggested by Stephen
  Boyd. This does not substantially change how any of the code works
  conceptually.
- Renamed new_string_stream() to alloc_string_stream() as suggested by
  Stephen Boyd.
- Made string-stream a KUnit managed object - based on a suggestion made
  by Stephen Boyd.
- Renamed kunit_new_stream() to alloc_kunit_stream() as suggested by
  Stephen Boyd.
- Removed the ability to set log level after allocating a kunit_stream,
  as suggested by Stephen Boyd.

[1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
[2] https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html#test-module
[3] https://lwn.net/Articles/790235/
[4] https://google.github.io/kunit-docs/third_party/kernel/docs/
[5] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2-rc4/v5

Comments

Frank Rowand June 20, 2019, 1:17 a.m. UTC | #1
Hi Brendan,

I am only responding to this because you asked me to in the v4 thread.

Thank you for evaluating my comments in the v4 thread and asking me to
comment on v5

On 6/17/19 1:25 AM, Brendan Higgins wrote:
> ## TL;DR
> 
> A not so quick follow-up to Stephen's suggestions on PATCH v4. Nothing
> that really changes any functionality or usage with the minor exception
> of a couple public functions that Stephen asked me to rename.
> Nevertheless, a good deal of clean up and fixes. See changes below.
> 
> As for our current status, right now we got Reviewed-bys on all patches
> except:
> 
> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>   list
> 
> However, it would probably be good to get reviews/acks from the
> subsystem maintainers on:
> 
> - [PATCH v5 06/18] kbuild: enable building KUnit
> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>   list
> - [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit
> - [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for
>   sysctl.c:proc_dointvec()
> - [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC
>   SYSCTL section
> 
> Other than that, I think we should be good to go.
> 
> One last thing, I updated the background to include my thoughts on KUnit
> vs. in kernel testing with kselftest in the background sections as
> suggested by Frank in the discussion on PATCH v2.
> 
> ## Background
> 
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> (however, KUnit still allows you to run tests on test machines or in VMs
> if you want[1]) and does not require tests to be written in userspace
> running on a host kernel. Additionally, KUnit is fast: From invocation
> to completion KUnit can run several dozen tests in under a second.
> Currently, the entire KUnit test suite for KUnit runs in under a second
> from the initial invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.
> 

I looked only at this section, as was specifically requested:

> ### But wait! Doesn't kselftest support in kernel testing?!
> 
> In a previous version of this patchset Frank pointed out that kselftest
> already supports writing a test that resides in the kernel using the
> test module feature[2]. LWN did a really great summary on this
> discussion here[3].
> 
> Kselftest has a feature that allows a test module to be loaded into a
> kernel using the kselftest framework; this does allow someone to write
> tests against kernel code not directly exposed to userland; however, it
> does not provide much of a framework around how to structure the tests.
> The kselftest test module feature just provides a header which has a
> standardized way of reporting test failures, 


> and then provides
> infrastructure to load and run the tests using the kselftest test
> harness.

The in-kernel tests can also be invoked at boot time if they are
configured (Kconfig) as in-kernel instead of as modules.  I did not
check how many of the tests have tri-state configuration to allow
this, but the few that I looked at did.

> 
> The kselftest test module does not seem to be opinionated at all in
> regards to how tests are structured, how they check for failures, how
> tests are organized. Even in the method it provides for reporting
> failures is pretty simple; it doesn't have any more advanced failure
> reporting or logging features. Given what's there, I think it is fair to
> say that it is not actually a framework, but a feature that makes it
> possible for someone to do some checks in kernel space.

I would call that description a little dismissive.  The set of in-kernel
tests that I looked like followed a common pattern and reported results
in a uniform manner.

> 
> Furthermore, kselftest test module has very few users. I checked for all
> the tests that use it using the following grep command:
> 
> grep -Hrn -e 'kselftest_module\.h'
> 
> and only got three results: lib/test_strscpy.c, lib/test_printf.c, and
> lib/test_bitmap.c.

You missed many tests.  I listed much more than that in the v4 thread, and
someone else also listed more in the v4 thread.


> 
> So despite kselftest test module's existence, there really is no feature
> overlap between kselftest and KUnit, save one: that you can use either
> to write an in-kernel test, but this is a very small feature in
> comparison to everything that KUnit allows you to do. KUnit is a full
> x-unit style unit testing framework, whereas kselftest looks a lot more
> like an end-to-end/functional testing framework, with a feature that
> makes it possible to write in-kernel tests.

The description does not give enough credit to what is in kselftest.

It does not matter whether KUnit provides additional things, relative
to kselftest.  The point I was making is that there appears to be
_some_ overlap between kselftest and KUnit, and if there is overlap
then it is worth considering whether the overlap can be unified instead
of duplicated.

I don't have a dog in this fight and the discussion in the v4 thread
went way off track.  Thus I am not going to get sucked back into a
pointless debate in this thread.

Thanks for adding this section to address the issue.

-Frank


> 
> ### What's so special about unit testing?
> 
> A unit test is supposed to test a single unit of code in isolation,
> hence the name. There should be no dependencies outside the control of
> the test; this means no external dependencies, which makes tests orders
> of magnitudes faster. Likewise, since there are no external dependencies,
> there are no hoops to jump through to run the tests. Additionally, this
> makes unit tests deterministic: a failing unit test always indicates a
> problem. Finally, because unit tests necessarily have finer granularity,
> they are able to test all code paths easily solving the classic problem
> of difficulty in exercising error handling code.
> 
> ### Is KUnit trying to replace other testing frameworks for the kernel?
> 
> No. Most existing tests for the Linux kernel are end-to-end tests, which
> have their place. A well tested system has lots of unit tests, a
> reasonable number of integration tests, and some end-to-end tests. KUnit
> is just trying to address the unit test space which is currently not
> being addressed.
> 
> ### More information on KUnit
> 
> There is a bunch of documentation near the end of this patch set that
> describes how to use KUnit and best practices for writing unit tests.
> For convenience I am hosting the compiled docs here[4].
> 
> Additionally for convenience, I have applied these patches to a
> branch[5]. The repo may be cloned with:
> git clone https://kunit.googlesource.com/linux
> This patchset is on the kunit/rfc/v5.2-rc4/v5 branch.
> 
> ## Changes Since Last Version
> 
> Aside from a couple public function renames, there isn't really anything
> in here that changes any functionality.
> 
> - Went through and fixed a couple of anti-patterns suggested by Stephen
>   Boyd. Things like:
>   - Dropping an else clause at the end of a function.
>   - Dropping the comma on the closing sentinel, `{}`, of a list.
> - Inlines a bunch of functions in the test case running logic in patch
>   01/18 to make it more readable as suggested by Stephen Boyd
> - Found and fixed bug in resource deallocation logic in patch 02/18. Bug
>   was discovered as a result of making a change suggested by Stephen
>   Boyd. This does not substantially change how any of the code works
>   conceptually.
> - Renamed new_string_stream() to alloc_string_stream() as suggested by
>   Stephen Boyd.
> - Made string-stream a KUnit managed object - based on a suggestion made
>   by Stephen Boyd.
> - Renamed kunit_new_stream() to alloc_kunit_stream() as suggested by
>   Stephen Boyd.
> - Removed the ability to set log level after allocating a kunit_stream,
>   as suggested by Stephen Boyd.
> 
> [1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
> [2] https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html#test-module
> [3] https://lwn.net/Articles/790235/
> [4] https://google.github.io/kunit-docs/third_party/kernel/docs/
> [5] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2-rc4/v5
>
shuah June 21, 2019, 2:59 p.m. UTC | #2
Hi Brendan,

On 6/19/19 7:17 PM, Frank Rowand wrote:
> Hi Brendan,
> 
> I am only responding to this because you asked me to in the v4 thread.
> 
> Thank you for evaluating my comments in the v4 thread and asking me to
> comment on v5
> 
> On 6/17/19 1:25 AM, Brendan Higgins wrote:
>> ## TL;DR
>>
>> A not so quick follow-up to Stephen's suggestions on PATCH v4. Nothing
>> that really changes any functionality or usage with the minor exception
>> of a couple public functions that Stephen asked me to rename.
>> Nevertheless, a good deal of clean up and fixes. See changes below.
>>
>> As for our current status, right now we got Reviewed-bys on all patches
>> except:
>>
>> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>>    list
>>
>> However, it would probably be good to get reviews/acks from the
>> subsystem maintainers on:
>>
>> - [PATCH v5 06/18] kbuild: enable building KUnit
>> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>>    list
>> - [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit
>> - [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for
>>    sysctl.c:proc_dointvec()
>> - [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC
>>    SYSCTL section
>>
>> Other than that, I think we should be good to go.
>>
>> One last thing, I updated the background to include my thoughts on KUnit
>> vs. in kernel testing with kselftest in the background sections as
>> suggested by Frank in the discussion on PATCH v2.
>>
>> ## Background
>>
>> This patch set proposes KUnit, a lightweight unit testing and mocking
>> framework for the Linux kernel.
>>
>> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
>> it does not require installing the kernel on a test machine or in a VM
>> (however, KUnit still allows you to run tests on test machines or in VMs
>> if you want[1]) and does not require tests to be written in userspace
>> running on a host kernel. Additionally, KUnit is fast: From invocation
>> to completion KUnit can run several dozen tests in under a second.
>> Currently, the entire KUnit test suite for KUnit runs in under a second
>> from the initial invocation (build time excluded).
>>
>> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
>> Googletest/Googlemock for C++. KUnit provides facilities for defining
>> unit test cases, grouping related test cases into test suites, providing
>> common infrastructure for running tests, mocking, spying, and much more.
>>
> 
> I looked only at this section, as was specifically requested:
> 
>> ### But wait! Doesn't kselftest support in kernel testing?!
>>
>> In a previous version of this patchset Frank pointed out that kselftest
>> already supports writing a test that resides in the kernel using the
>> test module feature[2]. LWN did a really great summary on this
>> discussion here[3].
>>
>> Kselftest has a feature that allows a test module to be loaded into a
>> kernel using the kselftest framework; this does allow someone to write
>> tests against kernel code not directly exposed to userland; however, it
>> does not provide much of a framework around how to structure the tests.
>> The kselftest test module feature just provides a header which has a
>> standardized way of reporting test failures,
> 
> 
>> and then provides
>> infrastructure to load and run the tests using the kselftest test
>> harness.
> 
> The in-kernel tests can also be invoked at boot time if they are
> configured (Kconfig) as in-kernel instead of as modules.  I did not
> check how many of the tests have tri-state configuration to allow
> this, but the few that I looked at did.
> 
>>
>> The kselftest test module does not seem to be opinionated at all in
>> regards to how tests are structured, how they check for failures, how
>> tests are organized. Even in the method it provides for reporting
>> failures is pretty simple; it doesn't have any more advanced failure
>> reporting or logging features. Given what's there, I think it is fair to
>> say that it is not actually a framework, but a feature that makes it
>> possible for someone to do some checks in kernel space.
> 
> I would call that description a little dismissive.  The set of in-kernel
> tests that I looked like followed a common pattern and reported results
> in a uniform manner.
> 
>>

I think I commented on this before. I agree with the statement that
there is no overlap between Kselftest and KUnit. I would like see this
removed. Kselftest module support supports use-cases KUnit won't be able
to. I can build an kernel with Kselftest test modules and use it in the
filed to load and run tests if I need to debug a problem and get data
from a system. I can't do that with KUnit.

In my mind, I am not viewing this as which is better. Kselftest and
KUnit both have their place in the kernel development process. It isn't
productive and/or necessary to comparing Kselftest and KUnit without a
good understanding of the problem spaces for each of these.

I would strongly recommend not making reference to Kselftest and talk
about what KUnit offers.

>> Furthermore, kselftest test module has very few users. I checked for all
>> the tests that use it using the following grep command:
>>
>> grep -Hrn -e 'kselftest_module\.h'
>>
>> and only got three results: lib/test_strscpy.c, lib/test_printf.c, and
>> lib/test_bitmap.c.
> 

Again, unnecessary. KUnit can't replace Kselftest module in the way
Kselftest module support can be used for debugging and gathering
information on system that might be in active use and not dedicated
to test and development alone. I wouldn't hesitate loading a Kselftest
test module on my laptop and running tests, but I wouldn't use KUnit
the same way.

Again, this is not a competition between which is better. Kselftest
and KUnit serve different needs and problem spaces.

Please redo this documentation to reflect that.

thanks,
-- Shuah
Theodore Y. Ts'o June 21, 2019, 6:13 p.m. UTC | #3
On Fri, Jun 21, 2019 at 08:59:48AM -0600, shuah wrote:
> > > ### But wait! Doesn't kselftest support in kernel testing?!
> > >
> > > ....
> 
> I think I commented on this before. I agree with the statement that
> there is no overlap between Kselftest and KUnit. I would like see this
> removed. Kselftest module support supports use-cases KUnit won't be able
> to. I can build an kernel with Kselftest test modules and use it in the
> filed to load and run tests if I need to debug a problem and get data
> from a system. I can't do that with KUnit.
> 
> In my mind, I am not viewing this as which is better. Kselftest and
> KUnit both have their place in the kernel development process. It isn't
> productive and/or necessary to comparing Kselftest and KUnit without a
> good understanding of the problem spaces for each of these.
>
> I would strongly recommend not making reference to Kselftest and talk
> about what KUnit offers.

Shuah,

Just to recall the history, this section of the FAQ was added to rebut
the ***very*** strong statements that Frank made that there was
overlap between Kselftest and Kunit, and that having too many ways for
kernel developers to do the identical thing was harmful (he said it
was too much of a burden on a kernel developer) --- and this was an
argument for not including Kunit in the upstream kernel.

If we're past that objection, then perhaps this section can be
dropped, but there's a very good reason why it was there.  I wouldn't
Brendan to be accused of ignoring feedback from those who reviewed his
patches.   :-)

						- Ted
shuah June 21, 2019, 7:20 p.m. UTC | #4
On 6/21/19 12:13 PM, Theodore Ts'o wrote:
> On Fri, Jun 21, 2019 at 08:59:48AM -0600, shuah wrote:
>>>> ### But wait! Doesn't kselftest support in kernel testing?!
>>>>
>>>> ....
>>
>> I think I commented on this before. I agree with the statement that
>> there is no overlap between Kselftest and KUnit. I would like see this
>> removed. Kselftest module support supports use-cases KUnit won't be able
>> to. I can build an kernel with Kselftest test modules and use it in the
>> filed to load and run tests if I need to debug a problem and get data
>> from a system. I can't do that with KUnit.
>>
>> In my mind, I am not viewing this as which is better. Kselftest and
>> KUnit both have their place in the kernel development process. It isn't
>> productive and/or necessary to comparing Kselftest and KUnit without a
>> good understanding of the problem spaces for each of these.
>>
>> I would strongly recommend not making reference to Kselftest and talk
>> about what KUnit offers.
> 
> Shuah,
> 
> Just to recall the history, this section of the FAQ was added to rebut
> the ***very*** strong statements that Frank made that there was
> overlap between Kselftest and Kunit, and that having too many ways for
> kernel developers to do the identical thing was harmful (he said it
> was too much of a burden on a kernel developer) --- and this was an
> argument for not including Kunit in the upstream kernel.
> 
> If we're past that objection, then perhaps this section can be
> dropped, but there's a very good reason why it was there.  I wouldn't
> Brendan to be accused of ignoring feedback from those who reviewed his
> patches.   :-)
> 

Agreed. I understand that this FAQ probably was needed at one time and
Brendan added it to address the concerns.

I think at some point we do need to have a document that outlines when
to KUnit and when to use Kselftest modules. I think one concern people
have is that if KUnit is perceived as a  replacement for Ksefltest
module, Kselftest module will be ignored leaving users without the
ability to build and run with Kselftest modules and load them on a need
basis to gather data on a systems that aren't dedicated strictly for
testing.

I am trying to move the conversation forward from KUnit vs. Kselftest
modules discussion to which problem areas each one addresses keeping
in mind that it is not about which is better. Kselftest and KUnit both
have their place in the kernel development process. We just have to be
clear on usage as we write tests for each.

thanks,
-- Shuah
Brendan Higgins June 21, 2019, 11:35 p.m. UTC | #5
On Wed, Jun 19, 2019 at 6:17 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> Hi Brendan,
>
> I am only responding to this because you asked me to in the v4 thread.

Yes, I did! Thank you, I appreciate it!

> Thank you for evaluating my comments in the v4 thread and asking me to
> comment on v5

Of course, I feel as though I ought to address any and all valid comments.

> On 6/17/19 1:25 AM, Brendan Higgins wrote:
> > ## TL;DR
> >
> > A not so quick follow-up to Stephen's suggestions on PATCH v4. Nothing
> > that really changes any functionality or usage with the minor exception
> > of a couple public functions that Stephen asked me to rename.
> > Nevertheless, a good deal of clean up and fixes. See changes below.
> >
> > As for our current status, right now we got Reviewed-bys on all patches
> > except:
> >
> > - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
> >   list
> >
> > However, it would probably be good to get reviews/acks from the
> > subsystem maintainers on:
> >
> > - [PATCH v5 06/18] kbuild: enable building KUnit
> > - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
> >   list
> > - [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit
> > - [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for
> >   sysctl.c:proc_dointvec()
> > - [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC
> >   SYSCTL section
> >
> > Other than that, I think we should be good to go.
> >
> > One last thing, I updated the background to include my thoughts on KUnit
> > vs. in kernel testing with kselftest in the background sections as
> > suggested by Frank in the discussion on PATCH v2.
> >
> > ## Background
> >
> > This patch set proposes KUnit, a lightweight unit testing and mocking
> > framework for the Linux kernel.
> >
> > Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> > it does not require installing the kernel on a test machine or in a VM
> > (however, KUnit still allows you to run tests on test machines or in VMs
> > if you want[1]) and does not require tests to be written in userspace
> > running on a host kernel. Additionally, KUnit is fast: From invocation
> > to completion KUnit can run several dozen tests in under a second.
> > Currently, the entire KUnit test suite for KUnit runs in under a second
> > from the initial invocation (build time excluded).
> >
> > KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> > Googletest/Googlemock for C++. KUnit provides facilities for defining
> > unit test cases, grouping related test cases into test suites, providing
> > common infrastructure for running tests, mocking, spying, and much more.
> >
>
> I looked only at this section, as was specifically requested:
>
> > ### But wait! Doesn't kselftest support in kernel testing?!
> >
> > In a previous version of this patchset Frank pointed out that kselftest
> > already supports writing a test that resides in the kernel using the
> > test module feature[2]. LWN did a really great summary on this
> > discussion here[3].
> >
> > Kselftest has a feature that allows a test module to be loaded into a
> > kernel using the kselftest framework; this does allow someone to write
> > tests against kernel code not directly exposed to userland; however, it
> > does not provide much of a framework around how to structure the tests.
> > The kselftest test module feature just provides a header which has a
> > standardized way of reporting test failures,
>
>
> > and then provides
> > infrastructure to load and run the tests using the kselftest test
> > harness.
>
> The in-kernel tests can also be invoked at boot time if they are
> configured (Kconfig) as in-kernel instead of as modules.  I did not
> check how many of the tests have tri-state configuration to allow
> this, but the few that I looked at did.
>
> >
> > The kselftest test module does not seem to be opinionated at all in
> > regards to how tests are structured, how they check for failures, how
> > tests are organized. Even in the method it provides for reporting
> > failures is pretty simple; it doesn't have any more advanced failure
> > reporting or logging features. Given what's there, I think it is fair to
> > say that it is not actually a framework, but a feature that makes it
> > possible for someone to do some checks in kernel space.
>
> I would call that description a little dismissive.  The set of in-kernel
> tests that I looked like followed a common pattern and reported results
> in a uniform manner.

I didn't mean to sound dismissive. I was only referring to what was
present in the actual header itself. There really isn't much there; it
provides a function which takes an expression, evaluates it,
increments a counter of all tests, and if false, prints out a warning;
also, it provides a module init which runs the user defined test
function called selftest(), and then prints the number of tests that
passed and the number of tests that failed; that's it. I was just
trying to make the point that it is pretty bare bones, and isn't
really much of a framework.

> >
> > Furthermore, kselftest test module has very few users. I checked for all
> > the tests that use it using the following grep command:
> >
> > grep -Hrn -e 'kselftest_module\.h'
> >
> > and only got three results: lib/test_strscpy.c, lib/test_printf.c, and
> > lib/test_bitmap.c.
>
> You missed many tests.  I listed much more than that in the v4 thread, and
> someone else also listed more in the v4 thread.

Oh, sorry, I forgot that more were listed in the thread.

> >
> > So despite kselftest test module's existence, there really is no feature
> > overlap between kselftest and KUnit, save one: that you can use either
> > to write an in-kernel test, but this is a very small feature in
> > comparison to everything that KUnit allows you to do. KUnit is a full
> > x-unit style unit testing framework, whereas kselftest looks a lot more
> > like an end-to-end/functional testing framework, with a feature that
> > makes it possible to write in-kernel tests.
>
> The description does not give enough credit to what is in kselftest.

You are right about me missing a number of the tests, but there really
is not much infrastructure in kselftest for this at all. It really
doesn't impose any structure of any kind other than that there must be
exactly one static function named selftest() that takes no arguments;
and then you use KSTM_CHECK_ZERO() to do some checks; that's it. It
doesn't have anything else in the actual kselftest module stuff.

>
> It does not matter whether KUnit provides additional things, relative
> to kselftest.  The point I was making is that there appears to be
> _some_ overlap between kselftest and KUnit, and if there is overlap
> then it is worth considering whether the overlap can be unified instead
> of duplicated.

I think I understand what you are saying, but the point I was trying
to make here is that it is so simplistic, it doesn't really
conceptually overlap since it is so limited in what structure and
features it provides. It's kind of like what Ted said previously about
how you have C so you can technically do whatever you want, but there
is nothing inherent to the kselftest test module that does any of
those things (other than what I mentioned above).

> I don't have a dog in this fight and the discussion in the v4 thread
> went way off track.  Thus I am not going to get sucked back into a
> pointless debate in this thread.

Sure, I don't want to debate the point further either (I had a hard
time understanding what the point was at the end myself).

Nevertheless, I do want to make sure that I addressed this because I
think you did indeed have a point that was worth addressing, and I
don't want to waste anyone's time in the future by not addressing it.

Nevertheless, like I said, I don't want to get too detailed on the
topic otherwise like Shuah suggests later, it might end up just
leading people to draw a comparison that doesn't need to be made, but
I also don't want to misrepresent anything. In anycase, I will follow
up on this point directly with Shuah.

> Thanks for adding this section to address the issue.

No need to thank me; that is what I felt is the correct thing to do. I
didn't address the point before and it caused you and other to spend a
lot of time debating the point.

Also, it looks like Shuah is asking me to drop the section. I will
discuss this point further there.

Thanks!

> -Frank
>
>
> >
> > ### What's so special about unit testing?
> >
> > A unit test is supposed to test a single unit of code in isolation,
> > hence the name. There should be no dependencies outside the control of
> > the test; this means no external dependencies, which makes tests orders
> > of magnitudes faster. Likewise, since there are no external dependencies,
> > there are no hoops to jump through to run the tests. Additionally, this
> > makes unit tests deterministic: a failing unit test always indicates a
> > problem. Finally, because unit tests necessarily have finer granularity,
> > they are able to test all code paths easily solving the classic problem
> > of difficulty in exercising error handling code.
> >
> > ### Is KUnit trying to replace other testing frameworks for the kernel?
> >
> > No. Most existing tests for the Linux kernel are end-to-end tests, which
> > have their place. A well tested system has lots of unit tests, a
> > reasonable number of integration tests, and some end-to-end tests. KUnit
> > is just trying to address the unit test space which is currently not
> > being addressed.
> >
> > ### More information on KUnit
> >
> > There is a bunch of documentation near the end of this patch set that
> > describes how to use KUnit and best practices for writing unit tests.
> > For convenience I am hosting the compiled docs here[4].
> >
> > Additionally for convenience, I have applied these patches to a
> > branch[5]. The repo may be cloned with:
> > git clone https://kunit.googlesource.com/linux
> > This patchset is on the kunit/rfc/v5.2-rc4/v5 branch.
> >
> > ## Changes Since Last Version
> >
> > Aside from a couple public function renames, there isn't really anything
> > in here that changes any functionality.
> >
> > - Went through and fixed a couple of anti-patterns suggested by Stephen
> >   Boyd. Things like:
> >   - Dropping an else clause at the end of a function.
> >   - Dropping the comma on the closing sentinel, `{}`, of a list.
> > - Inlines a bunch of functions in the test case running logic in patch
> >   01/18 to make it more readable as suggested by Stephen Boyd
> > - Found and fixed bug in resource deallocation logic in patch 02/18. Bug
> >   was discovered as a result of making a change suggested by Stephen
> >   Boyd. This does not substantially change how any of the code works
> >   conceptually.
> > - Renamed new_string_stream() to alloc_string_stream() as suggested by
> >   Stephen Boyd.
> > - Made string-stream a KUnit managed object - based on a suggestion made
> >   by Stephen Boyd.
> > - Renamed kunit_new_stream() to alloc_kunit_stream() as suggested by
> >   Stephen Boyd.
> > - Removed the ability to set log level after allocating a kunit_stream,
> >   as suggested by Stephen Boyd.
> >
> > [1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
> > [2] https://www.kernel.org/doc/html/latest/dev-tools/kselftest.html#test-module
> > [3] https://lwn.net/Articles/790235/
> > [4] https://google.github.io/kunit-docs/third_party/kernel/docs/
> > [5] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2-rc4/v5
> >
>
Brendan Higgins June 22, 2019, 12:54 a.m. UTC | #6
On Fri, Jun 21, 2019 at 12:21 PM shuah <shuah@kernel.org> wrote:
>
> On 6/21/19 12:13 PM, Theodore Ts'o wrote:
> > On Fri, Jun 21, 2019 at 08:59:48AM -0600, shuah wrote:
> >>>> ### But wait! Doesn't kselftest support in kernel testing?!
> >>>>
> >>>> ....
> >>
> >> I think I commented on this before. I agree with the statement that
> >> there is no overlap between Kselftest and KUnit. I would like see this
> >> removed. Kselftest module support supports use-cases KUnit won't be able
> >> to. I can build an kernel with Kselftest test modules and use it in the
> >> filed to load and run tests if I need to debug a problem and get data
> >> from a system. I can't do that with KUnit.

Sure, I think this point has been brought up a number of times before.
Maybe I didn't write this section well because, like Frank said, it
comes across as being critical of the Kselftest module support; that
wasn't my intention. I was speaking from the perspective that
Kselftest module support is just a feature of Kselftest, and not a
full framework like KUnit is (obviously Kselftest itself *is* a
framework, but a very small part of it is not).

It was obvious to me what Kselftest module support was intended for,
and it is not intended to cover the use case that KUnit is targeting.

> >> In my mind, I am not viewing this as which is better. Kselftest and
> >> KUnit both have their place in the kernel development process. It isn't
> >> productive and/or necessary to comparing Kselftest and KUnit without a
> >> good understanding of the problem spaces for each of these.

Again, I didn't mean to draw a comparison of which is better than the
other. I was just trying to point out that Kselftest module support
doesn't make sense as a stand alone unit testing framework, or really
a framework of any kind, despite how it might actually be used.

> >> I would strongly recommend not making reference to Kselftest and talk
> >> about what KUnit offers.

I can see your point. It seems that both you and Frank seem to think
that I drew a comparison between Kselftest and KUnit, which was
unintended. I probably should have spent more time editing this
section, but I can see the point of drawing the comparison itself
might invite this confusion.

> > Shuah,
> >
> > Just to recall the history, this section of the FAQ was added to rebut
> > the ***very*** strong statements that Frank made that there was
> > overlap between Kselftest and Kunit, and that having too many ways for
> > kernel developers to do the identical thing was harmful (he said it
> > was too much of a burden on a kernel developer) --- and this was an
> > argument for not including Kunit in the upstream kernel.

I don't think he was actually advocating that we don't include KUnit,
maybe playing devil's advocate; nevertheless, at the end, Frank seemed
to agree that there were valuable things that KUnit offered. I thought
he just wanted to make the point that I hadn't made the distinction
sufficiently clear in the cover letter, and other reviewers might get
confused in the future as well.

Additionally, it does look like people were trying to use Kselftest
module support to cover some things which really were trying to be
unit tests. I know this isn't really intended - everything looks like
a nail when you only have a hammer, which I think Frank was pointing
out furthers the above confusion.

In anycase, it sounds like I have, if anything, only made the
discussion even more confusing by adding this section; sorry about
that.

> > If we're past that objection, then perhaps this section can be
> > dropped, but there's a very good reason why it was there.  I wouldn't
> > Brendan to be accused of ignoring feedback from those who reviewed his
> > patches.   :-)
> >
>
> Agreed. I understand that this FAQ probably was needed at one time and
> Brendan added it to address the concerns.

I don't want to speak for Frank, but I don't think it was an objection
to KUnit itself, but rather an objection to not sufficiently
addressing the point about how they differ.

> I think at some point we do need to have a document that outlines when
> to KUnit and when to use Kselftest modules. I think one concern people
> have is that if KUnit is perceived as a  replacement for Ksefltest
> module, Kselftest module will be ignored leaving users without the
> ability to build and run with Kselftest modules and load them on a need
> basis to gather data on a systems that aren't dedicated strictly for
> testing.

I absolutely agree! I posed a suggestion here[1], which after I just
now searched for a link, I realize for some reason it didn't seem like
it reached a number of the mailing lists that I sent it to, so I
should probably resend it.

Anyway, a summary of what I suggested: We should start off by better
organizing Documentation/dev-tools/ and create a landing page that
groups the dev-tools by function according to what person is likely to
use them and for what. Eventually and specifically for Kselftest and
KUnit, I would like to have a testing guide for the kernel that
explains what testing procedure should look like and what to use and
when.

> I am trying to move the conversation forward from KUnit vs. Kselftest
> modules discussion to which problem areas each one addresses keeping
> in mind that it is not about which is better. Kselftest and KUnit both
> have their place in the kernel development process. We just have to be
> clear on usage as we write tests for each.

I think that is the right long term approach. I think a good place to
start, like I suggested above, is cleaning up
Documentation/dev-tools/, but I think that belongs in a (probably
several) follow-up patchset.

Frank, I believe your objection was mostly related to how KUnit is
presented specifically in the cover letter, and doesn't necessarily
deal with the intended use case. So I don't think that doing this,
especially doing this later, really addresses your concern. I don't
want to belabor the issue, but I would also rather not put words in
your mouth, what are your thoughts on the above?

I think my main concern moving forward on this point is that I am not
sure that I can address the debate that this section covers in a way
that is both sufficiently concise for a cover letter, but also doesn't
invite more potential confusion. My inclination at this point is to
drop it since I think the set of reviewers for this patchset has at
this point become fixed, and it seems that it will likely cause more
confusion rather than reduce it; also, I don't really think this will
be an issue for end users, especially once we have proper
documentation in place. Alternatively, I guess I could maybe address
the point elsewhere and refer to it in the cover letter? Or maybe just
put it at the end of the cover letter?

[1] https://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg05059.html
Luis Chamberlain June 26, 2019, 2:38 a.m. UTC | #7
On Wed, Jun 19, 2019 at 06:17:51PM -0700, Frank Rowand wrote:
> It does not matter whether KUnit provides additional things, relative
> to kselftest.  The point I was making is that there appears to be
> _some_ overlap between kselftest and KUnit, and if there is overlap
> then it is worth considering whether the overlap can be unified instead
> of duplicated.

From my experience as an author of several kselftests drivers, one
faily complex, and after reviewing the sysctl kunit test module, I
disagree with this.

Even if there were an overlap, I'd say let the best test harness win.
Just as we have different LSMs that do something similar.

But this is not about that though. Although both are testing code,
they do so in *very* different ways.

The design philosophy and architecture are fundamentally different. The
*only* thing I can think of where there is overlap is that both can test
similar code paths. Beyond that, the layout of how one itemizes tests
could be borrowed, but that would be up to each kselftest author to
decide, in fact some ksefltests do exist which follow similar pattern of
itemizing test cases and running them. Kunit just provides a proper
framework to do this easily but also with a focus on UML. This last
aspect makes kselftests fundamentally orthogonal from an architecture /
design perspective.

After careful review, I cannot personally identify what could be shared
at this point. Can you? If you did identify one part, how do you
recommend to share it?

  Luis
Brendan Higgins July 3, 2019, 11:40 p.m. UTC | #8
On Fri, Jun 21, 2019 at 5:54 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Fri, Jun 21, 2019 at 12:21 PM shuah <shuah@kernel.org> wrote:
> >
> > On 6/21/19 12:13 PM, Theodore Ts'o wrote:
> > > On Fri, Jun 21, 2019 at 08:59:48AM -0600, shuah wrote:
> > >>>> ### But wait! Doesn't kselftest support in kernel testing?!
> > >>>>
> > >>>> ....
> > >>
> > >> I think I commented on this before. I agree with the statement that
> > >> there is no overlap between Kselftest and KUnit. I would like see this
> > >> removed. Kselftest module support supports use-cases KUnit won't be able
> > >> to. I can build an kernel with Kselftest test modules and use it in the
> > >> filed to load and run tests if I need to debug a problem and get data
> > >> from a system. I can't do that with KUnit.
>
> Sure, I think this point has been brought up a number of times before.
> Maybe I didn't write this section well because, like Frank said, it
> comes across as being critical of the Kselftest module support; that
> wasn't my intention. I was speaking from the perspective that
> Kselftest module support is just a feature of Kselftest, and not a
> full framework like KUnit is (obviously Kselftest itself *is* a
> framework, but a very small part of it is not).
>
> It was obvious to me what Kselftest module support was intended for,
> and it is not intended to cover the use case that KUnit is targeting.
>
> > >> In my mind, I am not viewing this as which is better. Kselftest and
> > >> KUnit both have their place in the kernel development process. It isn't
> > >> productive and/or necessary to comparing Kselftest and KUnit without a
> > >> good understanding of the problem spaces for each of these.
>
> Again, I didn't mean to draw a comparison of which is better than the
> other. I was just trying to point out that Kselftest module support
> doesn't make sense as a stand alone unit testing framework, or really
> a framework of any kind, despite how it might actually be used.
>
> > >> I would strongly recommend not making reference to Kselftest and talk
> > >> about what KUnit offers.
>
> I can see your point. It seems that both you and Frank seem to think
> that I drew a comparison between Kselftest and KUnit, which was
> unintended. I probably should have spent more time editing this
> section, but I can see the point of drawing the comparison itself
> might invite this confusion.
>
> > > Shuah,
> > >
> > > Just to recall the history, this section of the FAQ was added to rebut
> > > the ***very*** strong statements that Frank made that there was
> > > overlap between Kselftest and Kunit, and that having too many ways for
> > > kernel developers to do the identical thing was harmful (he said it
> > > was too much of a burden on a kernel developer) --- and this was an
> > > argument for not including Kunit in the upstream kernel.
>
> I don't think he was actually advocating that we don't include KUnit,
> maybe playing devil's advocate; nevertheless, at the end, Frank seemed
> to agree that there were valuable things that KUnit offered. I thought
> he just wanted to make the point that I hadn't made the distinction
> sufficiently clear in the cover letter, and other reviewers might get
> confused in the future as well.
>
> Additionally, it does look like people were trying to use Kselftest
> module support to cover some things which really were trying to be
> unit tests. I know this isn't really intended - everything looks like
> a nail when you only have a hammer, which I think Frank was pointing
> out furthers the above confusion.
>
> In anycase, it sounds like I have, if anything, only made the
> discussion even more confusing by adding this section; sorry about
> that.
>
> > > If we're past that objection, then perhaps this section can be
> > > dropped, but there's a very good reason why it was there.  I wouldn't
> > > Brendan to be accused of ignoring feedback from those who reviewed his
> > > patches.   :-)
> > >
> >
> > Agreed. I understand that this FAQ probably was needed at one time and
> > Brendan added it to address the concerns.
>
> I don't want to speak for Frank, but I don't think it was an objection
> to KUnit itself, but rather an objection to not sufficiently
> addressing the point about how they differ.
>
> > I think at some point we do need to have a document that outlines when
> > to KUnit and when to use Kselftest modules. I think one concern people
> > have is that if KUnit is perceived as a  replacement for Ksefltest
> > module, Kselftest module will be ignored leaving users without the
> > ability to build and run with Kselftest modules and load them on a need
> > basis to gather data on a systems that aren't dedicated strictly for
> > testing.
>
> I absolutely agree! I posed a suggestion here[1], which after I just
> now searched for a link, I realize for some reason it didn't seem like
> it reached a number of the mailing lists that I sent it to, so I
> should probably resend it.
>
> Anyway, a summary of what I suggested: We should start off by better
> organizing Documentation/dev-tools/ and create a landing page that
> groups the dev-tools by function according to what person is likely to
> use them and for what. Eventually and specifically for Kselftest and
> KUnit, I would like to have a testing guide for the kernel that
> explains what testing procedure should look like and what to use and
> when.
>
> > I am trying to move the conversation forward from KUnit vs. Kselftest
> > modules discussion to which problem areas each one addresses keeping
> > in mind that it is not about which is better. Kselftest and KUnit both
> > have their place in the kernel development process. We just have to be
> > clear on usage as we write tests for each.
>
> I think that is the right long term approach. I think a good place to
> start, like I suggested above, is cleaning up
> Documentation/dev-tools/, but I think that belongs in a (probably
> several) follow-up patchset.
>
> Frank, I believe your objection was mostly related to how KUnit is
> presented specifically in the cover letter, and doesn't necessarily
> deal with the intended use case. So I don't think that doing this,
> especially doing this later, really addresses your concern. I don't
> want to belabor the issue, but I would also rather not put words in
> your mouth, what are your thoughts on the above?
>
> I think my main concern moving forward on this point is that I am not
> sure that I can address the debate that this section covers in a way
> that is both sufficiently concise for a cover letter, but also doesn't
> invite more potential confusion. My inclination at this point is to
> drop it since I think the set of reviewers for this patchset has at
> this point become fixed, and it seems that it will likely cause more
> confusion rather than reduce it; also, I don't really think this will

Since no one has objected to dropping the "### But wait! Doesn't
kselftest support in kernel testing?!" section in the past almost two
weeks, I am just going to continue on without it.

Cheers

> be an issue for end users, especially once we have proper
> documentation in place. Alternatively, I guess I could maybe address
> the point elsewhere and refer to it in the cover letter? Or maybe just
> put it at the end of the cover letter?
>
> [1] https://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg05059.html