mbox series

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

Message ID 20190814055108.214253-1-brendanhiggins@google.com (mailing list archive)
Headers show
Series kunit: introduce KUnit, the Linux kernel unit testing framework | expand

Message

Brendan Higgins Aug. 14, 2019, 5:50 a.m. UTC
## TL;DR

This revision addresses comments from Stephen and Bjorn Helgaas. Most
changes are pretty minor stuff that doesn't affect the API in anyway.
One significant change, however, is that I added support for freeing
kunit_resource managed resources before the test case is finished via
kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
KUnit on certain configurations (like the default one for x86, whoops).

Based on Stephen's feedback on the previous change, I think we are
pretty close. I am not expecting any significant changes from here on
out.

## 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 about 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.

### 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[2].

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

## Changes Since Last Version

- Added support for freeing kunit_resources (KUnit managed resources)
  via kunit_resource_destroy() as suggested by Stephen.
- Promoted WARN() after __noreturn function to BUG() in
  "[PATCH v13 09/18] kunit: test: add support for test abort" as
  suggested by Stephen.
- Dropped concept of death test since I am not actually using it yet as
  pointed out by Stephen.
- Replaced usage of warn_slowpath_fmt with WARN in kunit_do_assertion
  since warn_slowpath_fmt is not available on some build configurations,
  as pointed out by Bjorn.
- Lots of other minor changes suggested by Stephen.

[1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
[2] https://google.github.io/kunit-docs/third_party/kernel/docs/
[3] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.3/v13

Comments

Brendan Higgins Aug. 14, 2019, 10:03 a.m. UTC | #1
On Tue, Aug 13, 2019 at 10:52 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> ## TL;DR
>
> This revision addresses comments from Stephen and Bjorn Helgaas. Most
> changes are pretty minor stuff that doesn't affect the API in anyway.
> One significant change, however, is that I added support for freeing
> kunit_resource managed resources before the test case is finished via
> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> KUnit on certain configurations (like the default one for x86, whoops).
>
> Based on Stephen's feedback on the previous change, I think we are
> pretty close. I am not expecting any significant changes from here on
> out.

Stephen, it looks like you have just replied with "Reviewed-bys" on
all the remaining emails that you looked at. Is there anything else
that we are missing? Or is this ready for Shuah to apply?

[...]

Cheers!
Stephen Boyd Aug. 14, 2019, 5:23 p.m. UTC | #2
Quoting Brendan Higgins (2019-08-14 03:03:47)
> On Tue, Aug 13, 2019 at 10:52 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > ## TL;DR
> >
> > This revision addresses comments from Stephen and Bjorn Helgaas. Most
> > changes are pretty minor stuff that doesn't affect the API in anyway.
> > One significant change, however, is that I added support for freeing
> > kunit_resource managed resources before the test case is finished via
> > kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> > KUnit on certain configurations (like the default one for x86, whoops).
> >
> > Based on Stephen's feedback on the previous change, I think we are
> > pretty close. I am not expecting any significant changes from here on
> > out.
> 
> Stephen, it looks like you have just replied with "Reviewed-bys" on
> all the remaining emails that you looked at. Is there anything else
> that we are missing? Or is this ready for Shuah to apply?
> 

I think it's good to go! Thanks for the persistence.
Shuah Aug. 20, 2019, 5:24 p.m. UTC | #3
On 8/13/19 11:50 PM, Brendan Higgins wrote:
> ## TL;DR
> 
> This revision addresses comments from Stephen and Bjorn Helgaas. Most
> changes are pretty minor stuff that doesn't affect the API in anyway.
> One significant change, however, is that I added support for freeing
> kunit_resource managed resources before the test case is finished via
> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> KUnit on certain configurations (like the default one for x86, whoops).
> 
> Based on Stephen's feedback on the previous change, I think we are
> pretty close. I am not expecting any significant changes from here on
> out.
> 

Hi Brendan,

I found checkpatch errors in one or two patches. Can you fix those and
send v14.

thanks,
-- Shuah
Brendan Higgins Aug. 20, 2019, 6:24 p.m. UTC | #4
On Tue, Aug 20, 2019 at 11:24:45AM -0600, shuah wrote:
> On 8/13/19 11:50 PM, Brendan Higgins wrote:
> > ## TL;DR
> > 
> > This revision addresses comments from Stephen and Bjorn Helgaas. Most
> > changes are pretty minor stuff that doesn't affect the API in anyway.
> > One significant change, however, is that I added support for freeing
> > kunit_resource managed resources before the test case is finished via
> > kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> > KUnit on certain configurations (like the default one for x86, whoops).
> > 
> > Based on Stephen's feedback on the previous change, I think we are
> > pretty close. I am not expecting any significant changes from here on
> > out.
> > 
> 
> Hi Brendan,
> 
> I found checkpatch errors in one or two patches. Can you fix those and
> send v14.

Hi Shuah,

Are you refering to the following errors?

ERROR: Macros with complex values should be enclosed in parentheses
#144: FILE: include/kunit/test.h:456:
+#define KUNIT_BINARY_CLASS \
+       kunit_binary_assert, KUNIT_INIT_BINARY_ASSERT_STRUCT

ERROR: Macros with complex values should be enclosed in parentheses
#146: FILE: include/kunit/test.h:458:
+#define KUNIT_BINARY_PTR_CLASS \
+       kunit_binary_ptr_assert, KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT

These values should *not* be in parentheses. I am guessing checkpatch is
getting confused and thinks that these are complex expressions, when
they are not.

I ignored the errors since I figured checkpatch was complaining
erroneously.

I could refactor the code to remove these macros entirely, but I think
the code is cleaner with them.

What would you prefer I do?

NB: These macros are introduced in: "[PATCH v13 05/18] kunit: test: add
the concept of expectations"

Thanks!
Shuah Aug. 20, 2019, 7:08 p.m. UTC | #5
On 8/20/19 12:24 PM, Brendan Higgins wrote:
> On Tue, Aug 20, 2019 at 11:24:45AM -0600, shuah wrote:
>> On 8/13/19 11:50 PM, Brendan Higgins wrote:
>>> ## TL;DR
>>>
>>> This revision addresses comments from Stephen and Bjorn Helgaas. Most
>>> changes are pretty minor stuff that doesn't affect the API in anyway.
>>> One significant change, however, is that I added support for freeing
>>> kunit_resource managed resources before the test case is finished via
>>> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
>>> KUnit on certain configurations (like the default one for x86, whoops).
>>>
>>> Based on Stephen's feedback on the previous change, I think we are
>>> pretty close. I am not expecting any significant changes from here on
>>> out.
>>>
>>
>> Hi Brendan,
>>
>> I found checkpatch errors in one or two patches. Can you fix those and
>> send v14.
> 
> Hi Shuah,
> 
> Are you refering to the following errors?
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #144: FILE: include/kunit/test.h:456:
> +#define KUNIT_BINARY_CLASS \
> +       kunit_binary_assert, KUNIT_INIT_BINARY_ASSERT_STRUCT
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #146: FILE: include/kunit/test.h:458:
> +#define KUNIT_BINARY_PTR_CLASS \
> +       kunit_binary_ptr_assert, KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT
> 
> These values should *not* be in parentheses. I am guessing checkpatch is
> getting confused and thinks that these are complex expressions, when
> they are not.
> 
> I ignored the errors since I figured checkpatch was complaining
> erroneously.
> 
> I could refactor the code to remove these macros entirely, but I think
> the code is cleaner with them.
> 

Please do. I am not veru sure what value these macros add.

thanks,
-- Shuah
Brendan Higgins Aug. 20, 2019, 9:26 p.m. UTC | #6
On Tue, Aug 20, 2019 at 12:08 PM shuah <shuah@kernel.org> wrote:
>
> On 8/20/19 12:24 PM, Brendan Higgins wrote:
> > On Tue, Aug 20, 2019 at 11:24:45AM -0600, shuah wrote:
> >> On 8/13/19 11:50 PM, Brendan Higgins wrote:
> >>> ## TL;DR
> >>>
> >>> This revision addresses comments from Stephen and Bjorn Helgaas. Most
> >>> changes are pretty minor stuff that doesn't affect the API in anyway.
> >>> One significant change, however, is that I added support for freeing
> >>> kunit_resource managed resources before the test case is finished via
> >>> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> >>> KUnit on certain configurations (like the default one for x86, whoops).
> >>>
> >>> Based on Stephen's feedback on the previous change, I think we are
> >>> pretty close. I am not expecting any significant changes from here on
> >>> out.
> >>>
> >>
> >> Hi Brendan,
> >>
> >> I found checkpatch errors in one or two patches. Can you fix those and
> >> send v14.
> >
> > Hi Shuah,
> >
> > Are you refering to the following errors?
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #144: FILE: include/kunit/test.h:456:
> > +#define KUNIT_BINARY_CLASS \
> > +       kunit_binary_assert, KUNIT_INIT_BINARY_ASSERT_STRUCT
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #146: FILE: include/kunit/test.h:458:
> > +#define KUNIT_BINARY_PTR_CLASS \
> > +       kunit_binary_ptr_assert, KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT
> >
> > These values should *not* be in parentheses. I am guessing checkpatch is
> > getting confused and thinks that these are complex expressions, when
> > they are not.
> >
> > I ignored the errors since I figured checkpatch was complaining
> > erroneously.
> >
> > I could refactor the code to remove these macros entirely, but I think
> > the code is cleaner with them.
> >
>
> Please do. I am not veru sure what value these macros add.

Alright, I will have something for you later today.
Brendan Higgins Aug. 20, 2019, 11:23 p.m. UTC | #7
On Tue, Aug 20, 2019 at 2:26 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Tue, Aug 20, 2019 at 12:08 PM shuah <shuah@kernel.org> wrote:
> >
> > On 8/20/19 12:24 PM, Brendan Higgins wrote:
> > > On Tue, Aug 20, 2019 at 11:24:45AM -0600, shuah wrote:
> > >> On 8/13/19 11:50 PM, Brendan Higgins wrote:
> > >>> ## TL;DR
> > >>>
> > >>> This revision addresses comments from Stephen and Bjorn Helgaas. Most
> > >>> changes are pretty minor stuff that doesn't affect the API in anyway.
> > >>> One significant change, however, is that I added support for freeing
> > >>> kunit_resource managed resources before the test case is finished via
> > >>> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> > >>> KUnit on certain configurations (like the default one for x86, whoops).
> > >>>
> > >>> Based on Stephen's feedback on the previous change, I think we are
> > >>> pretty close. I am not expecting any significant changes from here on
> > >>> out.
> > >>>
> > >>
> > >> Hi Brendan,
> > >>
> > >> I found checkpatch errors in one or two patches. Can you fix those and
> > >> send v14.
> > >
> > > Hi Shuah,
> > >
> > > Are you refering to the following errors?
> > >
> > > ERROR: Macros with complex values should be enclosed in parentheses
> > > #144: FILE: include/kunit/test.h:456:
> > > +#define KUNIT_BINARY_CLASS \
> > > +       kunit_binary_assert, KUNIT_INIT_BINARY_ASSERT_STRUCT
> > >
> > > ERROR: Macros with complex values should be enclosed in parentheses
> > > #146: FILE: include/kunit/test.h:458:
> > > +#define KUNIT_BINARY_PTR_CLASS \
> > > +       kunit_binary_ptr_assert, KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT
> > >
> > > These values should *not* be in parentheses. I am guessing checkpatch is
> > > getting confused and thinks that these are complex expressions, when
> > > they are not.
> > >
> > > I ignored the errors since I figured checkpatch was complaining
> > > erroneously.
> > >
> > > I could refactor the code to remove these macros entirely, but I think
> > > the code is cleaner with them.
> > >
> >
> > Please do. I am not veru sure what value these macros add.
>
> Alright, I will have something for you later today.

I just sent a new revision with the fix.

Cheers
Shuah Aug. 21, 2019, 12:15 a.m. UTC | #8
On 8/20/19 5:23 PM, Brendan Higgins wrote:
> On Tue, Aug 20, 2019 at 2:26 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
>>
>> On Tue, Aug 20, 2019 at 12:08 PM shuah <shuah@kernel.org> wrote:
>>>
>>> On 8/20/19 12:24 PM, Brendan Higgins wrote:
>>>> On Tue, Aug 20, 2019 at 11:24:45AM -0600, shuah wrote:
>>>>> On 8/13/19 11:50 PM, Brendan Higgins wrote:
>>>>>> ## TL;DR
>>>>>>
>>>>>> This revision addresses comments from Stephen and Bjorn Helgaas. Most
>>>>>> changes are pretty minor stuff that doesn't affect the API in anyway.
>>>>>> One significant change, however, is that I added support for freeing
>>>>>> kunit_resource managed resources before the test case is finished via
>>>>>> kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
>>>>>> KUnit on certain configurations (like the default one for x86, whoops).
>>>>>>
>>>>>> Based on Stephen's feedback on the previous change, I think we are
>>>>>> pretty close. I am not expecting any significant changes from here on
>>>>>> out.
>>>>>>
>>>>>
>>>>> Hi Brendan,
>>>>>
>>>>> I found checkpatch errors in one or two patches. Can you fix those and
>>>>> send v14.
>>>>
>>>> Hi Shuah,
>>>>
>>>> Are you refering to the following errors?
>>>>
>>>> ERROR: Macros with complex values should be enclosed in parentheses
>>>> #144: FILE: include/kunit/test.h:456:
>>>> +#define KUNIT_BINARY_CLASS \
>>>> +       kunit_binary_assert, KUNIT_INIT_BINARY_ASSERT_STRUCT
>>>>
>>>> ERROR: Macros with complex values should be enclosed in parentheses
>>>> #146: FILE: include/kunit/test.h:458:
>>>> +#define KUNIT_BINARY_PTR_CLASS \
>>>> +       kunit_binary_ptr_assert, KUNIT_INIT_BINARY_PTR_ASSERT_STRUCT
>>>>
>>>> These values should *not* be in parentheses. I am guessing checkpatch is
>>>> getting confused and thinks that these are complex expressions, when
>>>> they are not.
>>>>
>>>> I ignored the errors since I figured checkpatch was complaining
>>>> erroneously.
>>>>
>>>> I could refactor the code to remove these macros entirely, but I think
>>>> the code is cleaner with them.
>>>>
>>>
>>> Please do. I am not veru sure what value these macros add.
>>
>> Alright, I will have something for you later today.
> 
> I just sent a new revision with the fix.
> 
> Cheers
> 

Thanks Brendan. I will get them in.

-- Shuah