| Message ID | 20190617082613.109131-1-brendanhiggins@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | kunit: introduce KUnit, the Linux kernel unit testing framework | expand |
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 >
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
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
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
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 > > >
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
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
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
## 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