Re: [PATCH v14 01/18] kunit: test: add KUnit test runner core

[Brendan Higgins]

On Fri, Aug 23, 2019 at 12:04 PM shuah <shuah@xxxxxxxxxx> wrote:
>
> On 8/23/19 12:56 PM, Brendan Higgins wrote:
> > On Fri, Aug 23, 2019 at 11:32 AM shuah <shuah@xxxxxxxxxx> wrote:
> >>
> >> On 8/23/19 11:54 AM, Brendan Higgins wrote:
> >>> On Fri, Aug 23, 2019 at 10:34 AM shuah <shuah@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 8/23/19 11:27 AM, Brendan Higgins wrote:
> >>>>> On Fri, Aug 23, 2019 at 10:05 AM shuah <shuah@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On 8/23/19 10:48 AM, Brendan Higgins wrote:
> >>>>>>> On Fri, Aug 23, 2019 at 8:33 AM shuah <shuah@xxxxxxxxxx> wrote:
> >>>>>>>>
> >>>>>>>> Hi Brendan,
> >>>>>>>>
> >>>>>>>> On 8/20/19 5:20 PM, Brendan Higgins wrote:
> >>>>>>>>> Add core facilities for defining unit tests; this provides a common way
> >>>>>>>>> to define test cases, functions that execute code which is under test
> >>>>>>>>> and determine whether the code under test behaves as expected; this also
> >>>>>>>>> provides a way to group together related test cases in test suites (here
> >>>>>>>>> we call them test_modules).
> >>>>>>>>>
> >>>>>>>>> Just define test cases and how to execute them for now; setting
> >>>>>>>>> expectations on code will be defined later.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>>       include/kunit/test.h | 179 ++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>       kunit/Kconfig        |  17 ++++
> >>>>>>>>>       kunit/Makefile       |   1 +
> >>>>>>>>>       kunit/test.c         | 191 +++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>       4 files changed, 388 insertions(+)
> >>>>>>>>>       create mode 100644 include/kunit/test.h
> >>>>>>>>>       create mode 100644 kunit/Kconfig
> >>>>>>>>>       create mode 100644 kunit/Makefile
> >>>>>>>>>       create mode 100644 kunit/test.c
> >>>>>>>>>
> >>>>>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 0000000000000..e0b34acb9ee4e
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/include/kunit/test.h
> >>>>>>>>> @@ -0,0 +1,179 @@
> >>>>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>>>>>>> +/*
> >>>>>>>>> + * Base unit test (KUnit) API.
> >>>>>>>>> + *
> >>>>>>>>> + * Copyright (C) 2019, Google LLC.
> >>>>>>>>> + * Author: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> >>>>>>>>> + */
> >>>>>>>>> +
> >>>>>>>>> +#ifndef _KUNIT_TEST_H
> >>>>>>>>> +#define _KUNIT_TEST_H
> >>>>>>>>> +
> >>>>>>>>> +#include <linux/types.h>
> >>>>>>>>> +
> >>>>>>>>> +struct kunit;
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>> + * struct kunit_case - represents an individual test case.
> >>>>>>>>> + * @run_case: the function representing the actual test case.
> >>>>>>>>> + * @name: the name of the test case.
> >>>>>>>>> + *
> >>>>>>>>> + * A test case is a function with the signature, ``void (*)(struct kunit *)``
> >>>>>>>>> + * that makes expectations (see KUNIT_EXPECT_TRUE()) about code under test. Each
> >>>>>>>>> + * test case is associated with a &struct kunit_suite and will be run after the
> >>>>>>>>> + * suite's init function and followed by the suite's exit function.
> >>>>>>>>> + *
> >>>>>>>>> + * A test case should be static and should only be created with the KUNIT_CASE()
> >>>>>>>>> + * macro; additionally, every array of test cases should be terminated with an
> >>>>>>>>> + * empty test case.
> >>>>>>>>> + *
> >>>>>>>>> + * Example:
> >>>>>>>>
> >>>>>>>> Can you fix these line continuations. It makes it very hard to read.
> >>>>>>>> Sorry for this late comment. These comments lines are longer than 80
> >>>>>>>> and wrap.
> >>>>>>>
> >>>>>>> None of the lines in this commit are over 80 characters in column
> >>>>>>> width. Some are exactly 80 characters (like above).
> >>>>>>>
> >>>>>>> My guess is that you are seeing the diff added text (+ ), which when
> >>>>>>> you add that to a line which is exactly 80 char in length ends up
> >>>>>>> being over 80 char in email. If you apply the patch you will see that
> >>>>>>> they are only 80 chars.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> There are several comment lines in the file that are way too long.
> >>>>>>>
> >>>>>>> Note that checkpatch also does not complain about any over 80 char
> >>>>>>> lines in this file.
> >>>>>>>
> >>>>>>> Sorry if I am misunderstanding what you are trying to tell me. Please
> >>>>>>> confirm either way.
> >>>>>>>
> >>>>>>
> >>>>>> WARNING: Avoid unnecessary line continuations
> >>>>>> #258: FILE: include/kunit/test.h:137:
> >>>>>> +                */                                                            \
> >>>>>>
> >>>>>> total: 0 errors, 2 warnings, 388 lines checked
> >>>>>
> >>>>> Ah, okay so you don't like the warning about the line continuation.
> >>>>> That's not because it is over 80 char, but because there is a line
> >>>>> continuation after a comment. I don't really see a way to get rid of
> >>>>> it without removing the comment from inside the macro.
> >>>>>
> >>>>> I put this TODO there in the first place a Luis' request, and I put it
> >>>>> in the body of the macro because this macro already had a kernel-doc
> >>>>> comment and I didn't think that an implementation detail TODO belonged
> >>>>> in the user documentation.
> >>>>>
> >>>>>> Go ahead fix these. It appears there are few lines that either longer
> >>>>>> than 80. In general, I keep them around 75, so it is easier read.
> >>>>>
> >>>>> Sorry, the above is the only checkpatch warning other than the
> >>>>> reminder to update the MAINTAINERS file.
> >>>>>
> >>>>> Are you saying you want me to go through and make all the lines fit in
> >>>>> 75 char column width? I hope not because that is going to be a pretty
> >>>>> substantial change to make.
> >>>>>
> >>>>
> >>>> There are two things with these comment lines. One is checkpatch
> >>>> complaining and the other is general readability.
> >>>
> >>> So for the checkpatch warning, do you want me to move the comment out
> >>> of the macro body into the kernel-doc comment? I don't really think it
> >>> is the right place for a comment of this nature, but I think it is
> >>> probably better than dropping it entirely (I don't see how else to do
> >>> it without just removing the comment entirely).
> >>>
> >>
> >> Don't drop the comments. It makes perfect sense to turn this into a
> >> kernel-doc comment.
> >
> > I am fine with that. I will do that in a subsequent revision once we
> > figure out the column limit issue.
> >
> >> We are going back forth on this a lot. I see several lines 81+ in
> >> this file. I am at 5.3-rc5 and my commit hooks aren't happy. I am
> >> fine with it if you want to convert these to kernel-doc comments.
> >> I think it makes perfect sense.
> >
> > Okay, so this is interesting. When I look at the applied patches in my
> > local repo, I don't see any 81+ lines. So it seems that something
> > interesting is going on here.
> >
> > To be clear (sorry for the stupid question) you are seeing the issue
> > after you applied the patch, and not in the patch file itself?
> >
>
> I am using my normal workflow. My pre-commit check is catching this.
> Just this patch.

Okay, *that* is super strange!

So I have lines in this patch (01/18) that are exactly 80 char wide
and I was thinking that it might be an off by one issue on either my
workflow or your workflow, but I have lines in other patches that are
exactly 80 char wide and our setups agree that they are fine, so I
really am not sure what's going on here.

It sounds like you are only seeing the issue in only a couple places,
do you mind calling out the specific lines that are problematic?

> All others are good other than the 9/18 BUG() issue.
> > Since we are still at OSS, would you mind if we meet up this afternoon
> > so I can see this issue you are seeing? I imagine we should get this
> > figured out pretty quickly.
> >
>
> Yeah. Would have been nice. I am not at oss today.

Dang.