Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Mon, Feb 18, 2019 at 2:56 PM Frank Rowand wrote:
>
> On 2/12/19 5:44 PM, Brendan Higgins wrote:
> > On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
> >>
> >> On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
> >> wrote:
> >>> ---
> >>> drivers/of/Kconfig|1 +
> >>> drivers/of/unittest.c | 1405 ++---
> >>> 2 files changed, 752 insertions(+), 654 deletions(-)
> >>>
> >
> >>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> >>> index 41b49716ac75f..a5ef44730ffdb 100644
> >>> --- a/drivers/of/unittest.c
> >>> +++ b/drivers/of/unittest.c
> >>> +
> >>> + KUNIT_EXPECT_EQ(test,
> >>> + of_property_match_string(np,
> >>> +"phandle-list-names",
> >>> +"first"),
> >>> + 0);
> >>> + KUNIT_EXPECT_EQ(test,
> >>> + of_property_match_string(np,
> >>> +"phandle-list-names",
> >>> +"second"),
> >>> + 1);
> >>
> >> Fewer lines on these would be better even if we go over 80 chars.
>
> Agreed. unittest.c already is a greater than 80 char file in general, and
> is a file that benefits from that.
>
Noted.
>
> > On the of_property_match_string(...), I have no opinion. I will do
> > whatever you like best.
> >
> > Nevertheless, as far as the KUNIT_EXPECT_*(...), I do have an opinion: I am
> > trying to establish a good, readable convention. Given an expect statement
> > structured as
> > ```
> > KUNIT_EXPECT_*(
> > test,
> > expect_arg_0, ..., expect_arg_n,
> > fmt_str, fmt_arg_0, ..., fmt_arg_n)
> > ```
> > where `test` is the `struct kunit` context argument, `expect_arg_{0, ...,
> > n}`
> > are the arguments the expectations is being made about (so in the above
> > example,
> > `of_property_match_string(...)` and `1`), and `fmt_*` is the optional format
> > string that comes at the end of some expectations.
> >
> > The pattern I had been trying to promote is the following:
> >
> > 1) If everything fits on 1 line, do that.
> > 2) If you must make a line split, prefer to keep `test` on its own line,
> > `expect_arg_{0, ..., n}` should be kept together, if possible, and the
> > format
> > string should follow the conventions already most commonly used with format
> > strings.
> > 3) If you must split up `expect_arg_{0, ..., n}` each argument should get
> > its
> > own line and should not share a line with either `test` or any `fmt_*`.
> >
> > The reason I care about this so much is because expectations should be
> > extremely easy to read; they are the most important part of a unit
> > test because they tell you what the test is verifying. I am not
> > married to the formatting I proposed above, but I want something that
> > will be extremely easy to identify the arguments that the expectation
> > is on. Maybe that means that I need to add some syntactic fluff to
> > make it clearer, I don't know, but this is definitely something we
> > need to get right, especially in the earliest examples.
>
> I will probably raise the ire of the kernel formatting rule makers by offering
> what I think is a _much_ more readable format __for this specific case__.
> In other words for drivers/of/unittest.c.
>
> If you can not make your mail window _very_ wide, or if this email has been
> line wrapped, this example will not be clear.
>
> Two possible formats:
>
>
> ### - version 1, as created by the patch series
>
> static void of_unittest_property_string(struct kunit *test)
> {
> const char *strings[4];
> struct device_node *np;
> int rc;
>
> np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>
> KUNIT_EXPECT_EQ(
> test,
> of_property_match_string(np, "phandle-list-names", "first"),
> 0);
> KUNIT_EXPECT_EQ(
> test,
> of_property_match_string(np, "phandle-list-names", "second"),
> 1);
> KUNIT_EXPECT_EQ(
> test,
> of_property_match_string(np, "phandle-list-names", "third"),
> 2);
> KUNIT_EXPECT_EQ_MSG(
> test,
> of_property_match_string(np, "phandle-list-names", "fourth"),
> -ENODATA,
> "unmatched string");
> KUNIT_EXPECT_EQ_MSG(
> test,
> of_property_match_string(np, "missing-property", "blah"),
> -EINVAL,
> "missing property");
> KUNIT_EXPECT_EQ_MSG(
> test,
> of_property_match_string(np, "empty-property", "blah"),
> -ENODATA,
> "empty property");
> KUNIT_EXPECT_EQ_MSG(
>
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On 2/12/19 5:44 PM, Brendan Higgins wrote:
> On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
>>
>> On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
>> wrote:
>>>
>>> Migrate tests without any cleanup, or modifying test logic in anyway to
>>> run under KUnit using the KUnit expectation and assertion API.
>>
>> Nice! You beat me to it. This is probably going to conflict with what
>> is in the DT tree for 4.21. Also, please Cc the DT list for
>> drivers/of/ changes.
>>
>> Looks good to me, but a few mostly formatting comments below.
>
> I just realized that we never talked about your other comments, and I
> still have some questions. (Sorry, it was the last thing I looked at
> while getting v4 ready.) No worries if you don't get to it before I
> send v4 out, I just didn't want you to think I was ignoring you.
>
>>
>>>
>>> Signed-off-by: Brendan Higgins
>>> ---
>>> drivers/of/Kconfig|1 +
>>> drivers/of/unittest.c | 1405 ++---
>>> 2 files changed, 752 insertions(+), 654 deletions(-)
>>>
>
>>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>>> index 41b49716ac75f..a5ef44730ffdb 100644
>>> --- a/drivers/of/unittest.c
>>> +++ b/drivers/of/unittest.c
>
>>> -
>>> -static void __init of_unittest_find_node_by_name(void)
>>> +static void of_unittest_find_node_by_name(struct kunit *test)
>>
>> Why do we have to drop __init everywhere? The tests run later?
>
>>From the standpoint of a unit test __init doesn't really make any
> sense, right? I know that right now we are running as part of a
> kernel, but the goal should be that a unit test is not part of a
> kernel and we just include what we need.
>
> Even so, that's the future. For now, I did not put the KUnit
> infrastructure in the .init section because I didn't think it belonged
> there. In practice, KUnit only knows how to run during the init phase
> of the kernel, but I don't think it should be restricted there. You
> should be able to run tests whenever you want because you should be
> able to test anything right? I figured any restriction on that is
> misleading and will potentially get in the way at worst, and
> unnecessary at best especially since people shouldn't build a
> production kernel with all kinds of unit tests inside.
>
>>
>>> {
>>> struct device_node *np;
>>> const char *options, *name;
>>>
>
>>>
>>>
>>> - np = of_find_node_by_path("/testcase-data/missing-path");
>>> - unittest(!np, "non-existent path returned node %pOF\n", np);
>>> + KUNIT_EXPECT_EQ_MSG(test,
>>> +
>>> of_find_node_by_path("/testcase-data/missing-path"),
>>> + NULL,
>>> + "non-existent path returned node %pOF\n", np);
>>
>> 1 tab indent would help with less vertical code (in general, not this
>> one so much).
>
> Will do.
>
>>
>>> of_node_put(np);
>>>
>>> - np = of_find_node_by_path("missing-alias");
>>> - unittest(!np, "non-existent alias returned node %pOF\n", np);
>>> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("missing-alias"),
>>> NULL,
>>> + "non-existent alias returned node %pOF\n", np);
>>> of_node_put(np);
>>>
>>> - np = of_find_node_by_path("testcase-alias/missing-path");
>>> - unittest(!np, "non-existent alias with relative path returned node
>>> %pOF\n", np);
>>> + KUNIT_EXPECT_EQ_MSG(test,
>>> +
>>> of_find_node_by_path("testcase-alias/missing-path"),
>>> + NULL,
>>> + "non-existent alias with relative path returned
>>> node %pOF\n",
>>> + np);
>>> of_node_put(np);
>>>
>
>>>
>>> -static void __init of_unittest_property_string(void)
>>> +static void of_unittest_property_string(struct kunit *test)
>>> {
>>> const char *strings[4];
>>> struct device_node *np;
>>> int rc;
>>>
>>> np =
>>> of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
>>> - if (!np) {
>>> - pr_err("No testcase data in device tree\n");
>>> - return;
>>> - }
>>> -
>>> - rc = of_property_match_string(np, "phandle-list-names", "first");
>>> - unittest(rc == 0, "first expected:0 got:%i\n", rc);
>>> - rc = of_property_match_string(np, "phandle-list-names", "second");
>>> - unittest(rc == 1, "second expected:1 got:%i\n", rc);
>>> - rc = of_property_match_string(np, "phandle-list-names", "third");
>>> - unittest(rc == 2, "third expected:2 got:%i\n", rc);
>>> - rc = of_property_match_string(np, "phandle-list-names", "fourth");
>>> - unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
>>> - rc = of_property_match_string(np, "missing-property", "blah");
>>> - unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
>>> - rc = of_property_match_string(np, "empty-property",
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Thu, Feb 14, 2019 at 12:10 PM Rob Herring wrote:
>
> On Tue, Feb 12, 2019 at 7:44 PM Brendan Higgins
> wrote:
> >
> > On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
> > >
> > > On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
> > > wrote:
> > > >
> > > > Migrate tests without any cleanup, or modifying test logic in anyway to
> > > > run under KUnit using the KUnit expectation and assertion API.
> > >
> > > Nice! You beat me to it. This is probably going to conflict with what
> > > is in the DT tree for 4.21. Also, please Cc the DT list for
> > > drivers/of/ changes.
> > >
> > > Looks good to me, but a few mostly formatting comments below.
> >
> > I just realized that we never talked about your other comments, and I
> > still have some questions. (Sorry, it was the last thing I looked at
> > while getting v4 ready.) No worries if you don't get to it before I
> > send v4 out, I just didn't want you to think I was ignoring you.
> >
> > >
> > > >
> > > > Signed-off-by: Brendan Higgins
> > > > ---
> > > > drivers/of/Kconfig|1 +
> > > > drivers/of/unittest.c | 1405 ++---
> > > > 2 files changed, 752 insertions(+), 654 deletions(-)
> > > >
> >
> > > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > > index 41b49716ac75f..a5ef44730ffdb 100644
> > > > --- a/drivers/of/unittest.c
> > > > +++ b/drivers/of/unittest.c
> >
> > > > -
> > > > -static void __init of_unittest_find_node_by_name(void)
> > > > +static void of_unittest_find_node_by_name(struct kunit *test)
> > >
> > > Why do we have to drop __init everywhere? The tests run later?
> >
> > From the standpoint of a unit test __init doesn't really make any
> > sense, right? I know that right now we are running as part of a
> > kernel, but the goal should be that a unit test is not part of a
> > kernel and we just include what we need.
>
> Well, the test only runs during boot and better to free the space when
> done with it. There was some desire to make it a kernel module and
> then we'd also need to get rid of __init too.
>
> > Even so, that's the future. For now, I did not put the KUnit
> > infrastructure in the .init section because I didn't think it belonged
> > there. In practice, KUnit only knows how to run during the init phase
> > of the kernel, but I don't think it should be restricted there. You
> > should be able to run tests whenever you want because you should be
> > able to test anything right? I figured any restriction on that is
> > misleading and will potentially get in the way at worst, and
> > unnecessary at best especially since people shouldn't build a
> > production kernel with all kinds of unit tests inside.
>
> More folks will run things if they can be enabled on production
> kernels. If size is the only issue, modules mitigate that. However,
> there's probably APIs to test which we don't want to export to
> modules.
>
> I think in general, we change things in the kernel when needed, not
> for something in the future. Changing __init is simple enough to do
> later.
>
> OTOH, things get copied and maybe this we don't want copied, so we can
> remove it if you want to.
Mmmm...I just realized that the patch I sent you the other day makes
this patch unhappy because unflatten_device_tree is in the .init
section. So I will need to fix that. I still think that the correct
course of action is to make KUnit non init. Luis pointed out in
another thread that to be 100% sure that everything will be properly
initialized, KUnit must be able to run after all initialization takes
place.
>
> >
> > > >
> > > > -static void __init of_unittest_property_string(void)
> > > > +static void of_unittest_property_string(struct kunit *test)
> > > > {
> > > > const char *strings[4];
> > > > struct device_node *np;
> > > > int rc;
> > > >
> > > > np =
> > > > of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> > > > - if (!np) {
> > > > - pr_err("No testcase data in device tree\n");
> > > > - return;
> > > > - }
> > > > -
> > > > - rc = of_property_match_string(np, "phandle-list-names",
> > > > "first");
> > > > - unittest(rc == 0, "first expected:0 got:%i\n", rc);
> > > > - rc = of_property_match_string(np, "phandle-list-names",
> > > > "second");
> > > > - unittest(rc == 1, "second expected:1 got:%i\n", rc);
> > > > - rc = of_property_match_string(np, "phandle-list-names",
> > > > "third");
> > > > - unittest(rc == 2, "third expected:2 got:%i\n", rc);
> > > > - rc = of_property_match_string(np, "phandle-list-names",
> > > > "fourth");
> > > > - unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
> > > > - rc = of_property_match_string(np, "missing-property", "blah");
> > > > - unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
> > > > - rc = of_property_match_string(np, "empty-property", "blah");
> > > > -
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Tue, Feb 12, 2019 at 7:44 PM Brendan Higgins
wrote:
>
> On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
> >
> > On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
> > wrote:
> > >
> > > Migrate tests without any cleanup, or modifying test logic in anyway to
> > > run under KUnit using the KUnit expectation and assertion API.
> >
> > Nice! You beat me to it. This is probably going to conflict with what
> > is in the DT tree for 4.21. Also, please Cc the DT list for
> > drivers/of/ changes.
> >
> > Looks good to me, but a few mostly formatting comments below.
>
> I just realized that we never talked about your other comments, and I
> still have some questions. (Sorry, it was the last thing I looked at
> while getting v4 ready.) No worries if you don't get to it before I
> send v4 out, I just didn't want you to think I was ignoring you.
>
> >
> > >
> > > Signed-off-by: Brendan Higgins
> > > ---
> > > drivers/of/Kconfig|1 +
> > > drivers/of/unittest.c | 1405 ++---
> > > 2 files changed, 752 insertions(+), 654 deletions(-)
> > >
>
> > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > index 41b49716ac75f..a5ef44730ffdb 100644
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
>
> > > -
> > > -static void __init of_unittest_find_node_by_name(void)
> > > +static void of_unittest_find_node_by_name(struct kunit *test)
> >
> > Why do we have to drop __init everywhere? The tests run later?
>
> From the standpoint of a unit test __init doesn't really make any
> sense, right? I know that right now we are running as part of a
> kernel, but the goal should be that a unit test is not part of a
> kernel and we just include what we need.
Well, the test only runs during boot and better to free the space when
done with it. There was some desire to make it a kernel module and
then we'd also need to get rid of __init too.
> Even so, that's the future. For now, I did not put the KUnit
> infrastructure in the .init section because I didn't think it belonged
> there. In practice, KUnit only knows how to run during the init phase
> of the kernel, but I don't think it should be restricted there. You
> should be able to run tests whenever you want because you should be
> able to test anything right? I figured any restriction on that is
> misleading and will potentially get in the way at worst, and
> unnecessary at best especially since people shouldn't build a
> production kernel with all kinds of unit tests inside.
More folks will run things if they can be enabled on production
kernels. If size is the only issue, modules mitigate that. However,
there's probably APIs to test which we don't want to export to
modules.
I think in general, we change things in the kernel when needed, not
for something in the future. Changing __init is simple enough to do
later.
OTOH, things get copied and maybe this we don't want copied, so we can
remove it if you want to.
>
> > >
> > > -static void __init of_unittest_property_string(void)
> > > +static void of_unittest_property_string(struct kunit *test)
> > > {
> > > const char *strings[4];
> > > struct device_node *np;
> > > int rc;
> > >
> > > np =
> > > of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> > > - if (!np) {
> > > - pr_err("No testcase data in device tree\n");
> > > - return;
> > > - }
> > > -
> > > - rc = of_property_match_string(np, "phandle-list-names", "first");
> > > - unittest(rc == 0, "first expected:0 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "second");
> > > - unittest(rc == 1, "second expected:1 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "third");
> > > - unittest(rc == 2, "third expected:2 got:%i\n", rc);
> > > - rc = of_property_match_string(np, "phandle-list-names", "fourth");
> > > - unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
> > > - rc = of_property_match_string(np, "missing-property", "blah");
> > > - unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
> > > - rc = of_property_match_string(np, "empty-property", "blah");
> > > - unittest(rc == -ENODATA, "empty property; rc=%i\n", rc);
> > > - rc = of_property_match_string(np, "unterminated-string", "blah");
> > > - unittest(rc == -EILSEQ, "unterminated string; rc=%i\n", rc);
> > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> > > +
> > > + KUNIT_EXPECT_EQ(test,
> > > + of_property_match_string(np,
> > > +"phandle-list-names",
> > > +"first"),
> > > + 0);
> > > + KUNIT_EXPECT_EQ(test,
> > > + of_property_match_string(np,
> > > +
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote:
>
> On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
> wrote:
> >
> > Migrate tests without any cleanup, or modifying test logic in anyway to
> > run under KUnit using the KUnit expectation and assertion API.
>
> Nice! You beat me to it. This is probably going to conflict with what
> is in the DT tree for 4.21. Also, please Cc the DT list for
> drivers/of/ changes.
>
> Looks good to me, but a few mostly formatting comments below.
I just realized that we never talked about your other comments, and I
still have some questions. (Sorry, it was the last thing I looked at
while getting v4 ready.) No worries if you don't get to it before I
send v4 out, I just didn't want you to think I was ignoring you.
>
> >
> > Signed-off-by: Brendan Higgins
> > ---
> > drivers/of/Kconfig|1 +
> > drivers/of/unittest.c | 1405 ++---
> > 2 files changed, 752 insertions(+), 654 deletions(-)
> >
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index 41b49716ac75f..a5ef44730ffdb 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > -
> > -static void __init of_unittest_find_node_by_name(void)
> > +static void of_unittest_find_node_by_name(struct kunit *test)
>
> Why do we have to drop __init everywhere? The tests run later?
From the standpoint of a unit test __init doesn't really make any
sense, right? I know that right now we are running as part of a
kernel, but the goal should be that a unit test is not part of a
kernel and we just include what we need.
Even so, that's the future. For now, I did not put the KUnit
infrastructure in the .init section because I didn't think it belonged
there. In practice, KUnit only knows how to run during the init phase
of the kernel, but I don't think it should be restricted there. You
should be able to run tests whenever you want because you should be
able to test anything right? I figured any restriction on that is
misleading and will potentially get in the way at worst, and
unnecessary at best especially since people shouldn't build a
production kernel with all kinds of unit tests inside.
>
> > {
> > struct device_node *np;
> > const char *options, *name;
> >
> >
> >
> > - np = of_find_node_by_path("/testcase-data/missing-path");
> > - unittest(!np, "non-existent path returned node %pOF\n", np);
> > + KUNIT_EXPECT_EQ_MSG(test,
> > +
> > of_find_node_by_path("/testcase-data/missing-path"),
> > + NULL,
> > + "non-existent path returned node %pOF\n", np);
>
> 1 tab indent would help with less vertical code (in general, not this
> one so much).
Will do.
>
> > of_node_put(np);
> >
> > - np = of_find_node_by_path("missing-alias");
> > - unittest(!np, "non-existent alias returned node %pOF\n", np);
> > + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("missing-alias"),
> > NULL,
> > + "non-existent alias returned node %pOF\n", np);
> > of_node_put(np);
> >
> > - np = of_find_node_by_path("testcase-alias/missing-path");
> > - unittest(!np, "non-existent alias with relative path returned node
> > %pOF\n", np);
> > + KUNIT_EXPECT_EQ_MSG(test,
> > +
> > of_find_node_by_path("testcase-alias/missing-path"),
> > + NULL,
> > + "non-existent alias with relative path returned
> > node %pOF\n",
> > + np);
> > of_node_put(np);
> >
> >
> > -static void __init of_unittest_property_string(void)
> > +static void of_unittest_property_string(struct kunit *test)
> > {
> > const char *strings[4];
> > struct device_node *np;
> > int rc;
> >
> > np =
> > of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> > - if (!np) {
> > - pr_err("No testcase data in device tree\n");
> > - return;
> > - }
> > -
> > - rc = of_property_match_string(np, "phandle-list-names", "first");
> > - unittest(rc == 0, "first expected:0 got:%i\n", rc);
> > - rc = of_property_match_string(np, "phandle-list-names", "second");
> > - unittest(rc == 1, "second expected:1 got:%i\n", rc);
> > - rc = of_property_match_string(np, "phandle-list-names", "third");
> > - unittest(rc == 2, "third expected:2 got:%i\n", rc);
> > - rc = of_property_match_string(np, "phandle-list-names", "fourth");
> > - unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
> > - rc = of_property_match_string(np, "missing-property", "blah");
> > - unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
> > - rc = of_property_match_string(np, "empty-property", "blah");
> > - unittest(rc == -ENODATA, "empty property; rc=%i\n", rc);
> > - rc =
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Dec 5, 2018 at 5:43 PM Brendan Higgins wrote: > > On Tue, Dec 4, 2018 at 5:41 AM Rob Herring wrote: > > > > On Mon, Dec 3, 2018 at 6:14 PM Brendan Higgins > > wrote: > > > > > > On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap > > > wrote: > > > > > > > > On 11/28/18 12:56 PM, Rob Herring wrote: > > > > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > > > >> index ad3fcad4d75b8..f309399deac20 100644 > > > > >> --- a/drivers/of/Kconfig > > > > >> +++ b/drivers/of/Kconfig > > > > >> @@ -15,6 +15,7 @@ if OF > > > > >> config OF_UNITTEST > > > > >> bool "Device Tree runtime unit tests" > > > > >> depends on !SPARC > > > > >> + depends on KUNIT > > > > > Unless KUNIT has depends, better to be a select here. > > > > > > > > That's just style or taste. I would prefer to use depends > > > > instead of select, but that's also just my preference. > > > > > > I prefer depends too, but Rob is the maintainer here. > > > > Well, we should be consistent, not the follow the whims of each maintainer. > > Sorry, I don't think that came out the way I meant it. I don't really > think we are consistent on this point across the kernel, and I don't > feel very strongly about the point, so I was just looking to follow > the path of least resistance. (I also just assumed Rob would keep us > consistent within drivers/of/.) I meant across unittests, we should be consistent. All unittests do either "depends on KUNIT" or "select KUNIT". The question I would ask is does KUNIT need to be user visible or is useful to enable without any unittests enabled? With depends, a user has 2 options to go enable vs. 1 with select. But if you want a global kill switch to turn off all unittests, then depends works better. > I figure if we are running unit tests from the test runner script or > from an automated system, you won't be hunting for dependencies for a > single test every time you want to run a test, so select doesn't make > it easier to configure in most imagined use cases. > > KUNIT hypothetically should not depend on anything, so select should > be safe to use. > > On the other hand, if we end up being wrong on this point and KUnit > gains widespread adoption, I would prefer not to be in a position > where I have to change a bunch of configs all over the kernel because > this example got copied and pasted. You'll be so happy that 100s of tests have been created using kunit, it won't be a big deal. :) In any case, I wouldn't spend more time on this. Rob ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Tue, Dec 4, 2018 at 5:41 AM Rob Herring wrote: > > On Mon, Dec 3, 2018 at 6:14 PM Brendan Higgins > wrote: > > > > On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap wrote: > > > > > > On 11/28/18 12:56 PM, Rob Herring wrote: > > > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > > >> index ad3fcad4d75b8..f309399deac20 100644 > > > >> --- a/drivers/of/Kconfig > > > >> +++ b/drivers/of/Kconfig > > > >> @@ -15,6 +15,7 @@ if OF > > > >> config OF_UNITTEST > > > >> bool "Device Tree runtime unit tests" > > > >> depends on !SPARC > > > >> + depends on KUNIT > > > > Unless KUNIT has depends, better to be a select here. > > > > > > That's just style or taste. I would prefer to use depends > > > instead of select, but that's also just my preference. > > > > I prefer depends too, but Rob is the maintainer here. > > Well, we should be consistent, not the follow the whims of each maintainer. Sorry, I don't think that came out the way I meant it. I don't really think we are consistent on this point across the kernel, and I don't feel very strongly about the point, so I was just looking to follow the path of least resistance. (I also just assumed Rob would keep us consistent within drivers/of/.) I figure if we are running unit tests from the test runner script or from an automated system, you won't be hunting for dependencies for a single test every time you want to run a test, so select doesn't make it easier to configure in most imagined use cases. KUNIT hypothetically should not depend on anything, so select should be safe to use. On the other hand, if we end up being wrong on this point and KUnit gains widespread adoption, I would prefer not to be in a position where I have to change a bunch of configs all over the kernel because this example got copied and pasted. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On 11/28/18 11:36 AM, Brendan Higgins wrote: > Migrate tests without any cleanup, or modifying test logic in anyway to > run under KUnit using the KUnit expectation and assertion API. > > Signed-off-by: Brendan Higgins > --- > drivers/of/Kconfig|1 + > drivers/of/unittest.c | 1405 ++--- > 2 files changed, 752 insertions(+), 654 deletions(-) < snip > I am travelling and will not have an opportunity to properly review this patch, patch 18, or patch 19 until next week. -Frank ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Mon, Dec 3, 2018 at 6:14 PM Brendan Higgins wrote: > > On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap wrote: > > > > On 11/28/18 12:56 PM, Rob Herring wrote: > > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > > >> index ad3fcad4d75b8..f309399deac20 100644 > > >> --- a/drivers/of/Kconfig > > >> +++ b/drivers/of/Kconfig > > >> @@ -15,6 +15,7 @@ if OF > > >> config OF_UNITTEST > > >> bool "Device Tree runtime unit tests" > > >> depends on !SPARC > > >> + depends on KUNIT > > > Unless KUNIT has depends, better to be a select here. > > > > That's just style or taste. I would prefer to use depends > > instead of select, but that's also just my preference. > > I prefer depends too, but Rob is the maintainer here. Well, we should be consistent, not the follow the whims of each maintainer. Rob ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Thu, Nov 29, 2018 at 4:40 PM Randy Dunlap wrote: > > On 11/28/18 12:56 PM, Rob Herring wrote: > >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > >> index ad3fcad4d75b8..f309399deac20 100644 > >> --- a/drivers/of/Kconfig > >> +++ b/drivers/of/Kconfig > >> @@ -15,6 +15,7 @@ if OF > >> config OF_UNITTEST > >> bool "Device Tree runtime unit tests" > >> depends on !SPARC > >> + depends on KUNIT > > Unless KUNIT has depends, better to be a select here. > > That's just style or taste. I would prefer to use depends > instead of select, but that's also just my preference. I prefer depends too, but Rob is the maintainer here. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Nov 28, 2018 at 12:56 PM Rob Herring wrote: > > On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins > wrote: > > > > Migrate tests without any cleanup, or modifying test logic in anyway to > > run under KUnit using the KUnit expectation and assertion API. > > Nice! You beat me to it. This is probably going to conflict with what > is in the DT tree for 4.21. Also, please Cc the DT list for > drivers/of/ changes. Oh, I thought you were asking me to do it :-) In any case, I am happy to. Oh yeah, sorry about not CC'ing the list. Cheers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On 11/28/18 12:56 PM, Rob Herring wrote: >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig >> index ad3fcad4d75b8..f309399deac20 100644 >> --- a/drivers/of/Kconfig >> +++ b/drivers/of/Kconfig >> @@ -15,6 +15,7 @@ if OF >> config OF_UNITTEST >> bool "Device Tree runtime unit tests" >> depends on !SPARC >> + depends on KUNIT > Unless KUNIT has depends, better to be a select here. That's just style or taste. I would prefer to use depends instead of select, but that's also just my preference. -- ~Randy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v3 17/19] of: unittest: migrate tests to run on KUnit
Migrate tests without any cleanup, or modifying test logic in anyway to
run under KUnit using the KUnit expectation and assertion API.
Signed-off-by: Brendan Higgins
---
drivers/of/Kconfig|1 +
drivers/of/unittest.c | 1405 ++---
2 files changed, 752 insertions(+), 654 deletions(-)
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index ad3fcad4d75b8..f309399deac20 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -15,6 +15,7 @@ if OF
config OF_UNITTEST
bool "Device Tree runtime unit tests"
depends on !SPARC
+ depends on KUNIT
select IRQ_DOMAIN
select OF_EARLY_FLATTREE
select OF_RESOLVE
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 41b49716ac75f..a5ef44730ffdb 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -26,186 +26,187 @@
#include
+#include
+
#include "of_private.h"
-static struct unittest_results {
- int passed;
- int failed;
-} unittest_results;
-
-#define unittest(result, fmt, ...) ({ \
- bool failed = !(result); \
- if (failed) { \
- unittest_results.failed++; \
- pr_err("FAIL %s():%i " fmt, __func__, __LINE__, ##__VA_ARGS__);
\
- } else { \
- unittest_results.passed++; \
- pr_debug("pass %s():%i\n", __func__, __LINE__); \
- } \
- failed; \
-})
-
-static void __init of_unittest_find_node_by_name(void)
+static void of_unittest_find_node_by_name(struct kunit *test)
{
struct device_node *np;
const char *options, *name;
np = of_find_node_by_path("/testcase-data");
name = kasprintf(GFP_KERNEL, "%pOF", np);
- unittest(np && !strcmp("/testcase-data", name),
- "find /testcase-data failed\n");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
+ KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
+ "find /testcase-data failed\n");
of_node_put(np);
kfree(name);
/* Test if trailing '/' works */
- np = of_find_node_by_path("/testcase-data/");
- unittest(!np, "trailing '/' on /testcase-data/ should fail\n");
+ KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"), NULL,
+ "trailing '/' on /testcase-data/ should fail\n");
np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
name = kasprintf(GFP_KERNEL, "%pOF", np);
- unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", name),
- "find /testcase-data/phandle-tests/consumer-a failed\n");
+ KUNIT_EXPECT_STREQ_MSG(test,
+ "/testcase-data/phandle-tests/consumer-a", name,
+ "find /testcase-data/phandle-tests/consumer-a
failed\n");
of_node_put(np);
kfree(name);
np = of_find_node_by_path("testcase-alias");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
name = kasprintf(GFP_KERNEL, "%pOF", np);
- unittest(np && !strcmp("/testcase-data", name),
- "find testcase-alias failed\n");
+ KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
+ "find testcase-alias failed\n");
of_node_put(np);
kfree(name);
/* Test if trailing '/' works on aliases */
- np = of_find_node_by_path("testcase-alias/");
- unittest(!np, "trailing '/' on testcase-alias/ should fail\n");
+ KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"), NULL,
+ "trailing '/' on testcase-alias/ should fail\n");
np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
name = kasprintf(GFP_KERNEL, "%pOF", np);
- unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", name),
- "find testcase-alias/phandle-tests/consumer-a failed\n");
+ KUNIT_EXPECT_STREQ_MSG(test,
+ "/testcase-data/phandle-tests/consumer-a", name,
+ "find testcase-alias/phandle-tests/consumer-a
failed\n");
of_node_put(np);
kfree(name);
- np = of_find_node_by_path("/testcase-data/missing-path");
- unittest(!np, "non-existent path returned node %pOF\n", np);
+ KUNIT_EXPECT_EQ_MSG(test,
+ of_find_node_by_path("/testcase-data/missing-path"),
+ NULL,
+ "non-existent path returned node %pOF\n", np);
of_node_put(np);
- np = of_find_node_by_path("missing-alias");
- unittest(!np, "non-existent alias returned node %pOF\n", np);
+ KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("missing-alias"), NULL,
+ "non-existent alias returned node %pOF\n", np);
Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit
On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
wrote:
>
> Migrate tests without any cleanup, or modifying test logic in anyway to
> run under KUnit using the KUnit expectation and assertion API.
Nice! You beat me to it. This is probably going to conflict with what
is in the DT tree for 4.21. Also, please Cc the DT list for
drivers/of/ changes.
Looks good to me, but a few mostly formatting comments below.
>
> Signed-off-by: Brendan Higgins
> ---
> drivers/of/Kconfig|1 +
> drivers/of/unittest.c | 1405 ++---
> 2 files changed, 752 insertions(+), 654 deletions(-)
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index ad3fcad4d75b8..f309399deac20 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -15,6 +15,7 @@ if OF
> config OF_UNITTEST
> bool "Device Tree runtime unit tests"
> depends on !SPARC
> + depends on KUNIT
Unless KUNIT has depends, better to be a select here.
> select IRQ_DOMAIN
> select OF_EARLY_FLATTREE
> select OF_RESOLVE
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 41b49716ac75f..a5ef44730ffdb 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -26,186 +26,187 @@
>
> #include
>
> +#include
> +
> #include "of_private.h"
>
> -static struct unittest_results {
> - int passed;
> - int failed;
> -} unittest_results;
> -
> -#define unittest(result, fmt, ...) ({ \
> - bool failed = !(result); \
> - if (failed) { \
> - unittest_results.failed++; \
> - pr_err("FAIL %s():%i " fmt, __func__, __LINE__,
> ##__VA_ARGS__); \
> - } else { \
> - unittest_results.passed++; \
> - pr_debug("pass %s():%i\n", __func__, __LINE__); \
> - } \
> - failed; \
> -})
> -
> -static void __init of_unittest_find_node_by_name(void)
> +static void of_unittest_find_node_by_name(struct kunit *test)
Why do we have to drop __init everywhere? The tests run later?
> {
> struct device_node *np;
> const char *options, *name;
>
> np = of_find_node_by_path("/testcase-data");
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find /testcase-data failed\n");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> + "find /testcase-data failed\n");
> of_node_put(np);
> kfree(name);
>
> /* Test if trailing '/' works */
> - np = of_find_node_by_path("/testcase-data/");
> - unittest(!np, "trailing '/' on /testcase-data/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"),
> NULL,
> + "trailing '/' on /testcase-data/ should fail\n");
>
> np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a",
> name),
> - "find /testcase-data/phandle-tests/consumer-a failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test,
> + "/testcase-data/phandle-tests/consumer-a",
> name,
> + "find /testcase-data/phandle-tests/consumer-a
> failed\n");
> of_node_put(np);
> kfree(name);
>
> np = of_find_node_by_path("testcase-alias");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find testcase-alias failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> + "find testcase-alias failed\n");
> of_node_put(np);
> kfree(name);
>
> /* Test if trailing '/' works on aliases */
> - np = of_find_node_by_path("testcase-alias/");
> - unittest(!np, "trailing '/' on testcase-alias/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"),
> NULL,
> + "trailing '/' on testcase-alias/ should fail\n");
>
> np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a",
> name),
> - "find testcase-alias/phandle-tests/consumer-a failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test,
> + "/testcase-data/phandle-tests/consumer-a",
> name,
> + "find testcase-alias/phandle-tests/consumer-a
> failed\n");
> of_node_put(np);
> kfree(name);
>
> - np =
