Linux-NVDIMM Archive on lore.kernel.org
 help / color / Atom feed
From: Frank Rowand <frowand.list@gmail.com>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: brakmo@fb.com, dri-devel <dri-devel@lists.freedesktop.org>,
	linux-kselftest@vger.kernel.org, shuah@kernel.org,
	Rob Herring <robh@kernel.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>,
	Richard Weinberger <richard@nod.at>,
	Knut Omang <knut.omang@oracle.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Joel Stanley <joel@jms.id.au>, Jeff Dike <jdike@addtoit.com>,
	"Bird," Timothy" <Tim.Bird@sony.com>,
	Kees Cook <keescook@google.com>," linux-um@lists.infradead.org,
	Steven Rostedt <rostedt@goodmis.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	kunit-dev@googlegroups.com, Greg KH <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Daniel Vetter <daniel@ffwll.ch>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Joe Perches <joe@perches.com>,
	Kevin Hilman <khilman@baylibre.com>
Subject: Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest
Date: Mon, 18 Feb 2019 14:25:59 -0800
Message-ID: <d9f7e000-4cac-a35a-3ff9-60130e12ebea@gmail.com> (raw)
In-Reply-To: <CAFd5g44NTZoSAJPMvXP2xvJgn7m5QoV-KJu2AMrr67+eL+CKrQ@mail.gmail.com>

On 2/15/19 2:56 AM, Brendan Higgins wrote:
> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> Hi Brendan,
>>>>>>
>>>>>> On 11/28/18 11:36 AM, Brendan Higgins wrote:
>>>>>>> Split out a couple of test cases that these features in base.c from the
>>>>>>> unittest.c monolith. The intention is that we will eventually split out
>>>>>>> all test cases and group them together based on what portion of device
>>>>>>> tree they test.
>>>>>>
>>>>>> Why does splitting this file apart improve the implementation?
>>>>>
>>>>> This is in preparation for patch 19/19 and other hypothetical future
>>>>> patches where test cases are split up and grouped together by what
>>>>> portion of DT they test (for example the parsing tests and the
>>>>> platform/device tests would probably go separate files as well). This
>>>>> patch by itself does not do anything useful, but I figured it made
>>>>> patch 19/19 (and, if you like what I am doing, subsequent patches)
>>>>> easier to review.
>>>>
>>>> I do not see any value in splitting the devicetree tests into
>>>> multiple files.
>>>>
>>>> Please help me understand what the benefits of such a split are.
>>
>> Note that my following comments are specific to the current devicetree
>> unittests, and may not apply to the general case of unit tests in other
>> subsystems.
>>
> Note taken.
>>
>>> Sorry, I thought it made sense in context of what I am doing in the
>>> following patch. All I am trying to do is to provide an effective way
>>> of grouping test cases. To be clear, the idea, assuming you agree, is
>>
>> Looking at _just_ the first few fragments of the following patch, the
>> change is to break down a moderate size function of related tests,
>> of_unittest_find_node_by_name(), into a lot of extremely small functions.
> 
> Hmm...I wouldn't call that a moderate function. By my standards those
> functions are pretty large. In any case, I want to limit the
> discussion to specifically what a test case should look like, and the
> general consensus outside of the kernel is that unit test cases should
> be very very small. The reason is that each test case is supposed to> test one specific property; it should be obvious what that property
> is; and it should be obvious what is needed to exercise that property.

That is a valid model and philosophy of unit test design.

It is not a model that the devicetree unit tests can be shoe horned
into.  Sort of...  In a sense, the existing devicetree unit tests
already to that, if you consider each unittest() (and sometime a few
lines of code that creates the result that unittest() checks) to be a separate
unit test.  But the kunit model does not consider the sort of
equivalent KUNIT_EXPECT_EQ(), etc, to be a unit test, the unit test
in kunit would be KUNIT_CASE().  Although it is a little confusing to
me that the initialization and clean up on exit occur one level
higher than KUNIT_CASE(), in struct kunit_module.  I think the
confusion is just a matter of slight conflict in the documentation
(btw, the documents where very helpful for me to understand the
overall concepts and model).


>> Then to find the execution order of the many small functions requires
>> finding the array of_test_find_node_by_name_cases[].  Then I have to
> 
> Execution order shouldn't matter. Each test case should be totally
> hermetic. Obviously in this case we depend on the preceeding test case
> to clean up properly, but that is something I am working on.

But the order _does_ matter for the devicetree unit tests.

That is one of the problems.  The devicetree unit tests are not small,
independent tests.  Some of the tests change state in a way that
following tests depend upon.

The design documents also mention that each unit test should have
a pre-test initialization, and a post-test cleanup to remove the
results of the initialization.

The devicetree unit tests have a large, intrusive initialization.
Once again, not a good fit for this model.

The devicetree unit tests also have an undocumented (and not at all
obvious) need to leave state changed in some cases after the test
completes.  There are cases where the way that I fully validate
the success of the tests is to examine the state of the live
devicetree via /proc/devicetree/. Ideally, this would be done by
a script or a program, but creating that is not near the top of
my todo list.


>> chase off into the kunit test runner core, where I find that the set
>> of tests in of_test_find_node_by_name_cases[] is processed by a
>> late_initcall().  So now the order of the various test groupings,
> 
> That's fair. You are not the only one to complain about that. The
> late_initcall is a hack which I plan on replacing shortly (and yes I
> know that me planning on doing something doesn't mean much in this
> discussion, but that's what I got); regardless, order shouldn't
> matter.

But again, it does.


>> declared via module_test(), are subject to the fragile orderings
>> of initcalls.
>>
>> There are ordering dependencies within the devicetree unittests.
> 
> There is now in the current devicetree unittests, but, if I may be so
> bold, that is something that I would like to fix.
> 
>>
>> I do not like breaking the test cases down into such small atoms.
>>
>> I do not see any value __for devicetree unittests__ of having
>> such small atoms.
> 
> I imagine it probably makes less sense in the context of a strict
> dependency order, but that is something that I want to do away with.
> Ideally, when you look at a test case you shouldn't need to think
> about anything other than the code under test and the test case
> itself; so in my universe, a smaller test case should mean less you
> need to think about.

For the general case, I think that is an excellent model.


> I don't want to get hung up on size too much because I don't think
> this is what it is really about. I think you and I can agree that a
> test should be as simple and complete as possible. The ideal test
> should cover all behavior, and should be obviously correct (since
> otherwise we would have to test the test too). Obviously, these two
> goals are at odds, so the compromise I attempt to make is to make a
> bunch of test cases which are separately simple enough to be obviously
> correct at first glance, and the sum total of all the tests provides
> the necessary coverage. Additionally, because each test case is
> independent of every other test case, they can be reasoned about
> individually, and it is not necessary to reason about them as a group.
> Hypothetically, this should give you the best of both worlds.
> 
> So even if I failed in execution, I think the principle is good.
> 
>>
>> It makes it harder for me to read the source of the tests and
>> understand the order they will execute.  It also makes it harder
>> for me to read through the actual tests (in this example the
>> tests that are currently grouped in of_unittest_find_node_by_name())
>> because of all the extra function headers injected into the
>> existing single function to break it apart into many smaller
>> functions.
> 
> Well now the same groups are expressed as test modules, it's just a
> collection of closely related test cases, but they are grouped
> together for just that reason. Nevertheless, I argue this is superior
> to grouping them together in a function, because a test module
> (elsewhere called a test suite) relates test cases together, but makes
> it clear that they are still logically independent, two test cases in
> a suite should run completely independently of each other.

That is missing my point.  Converting to the kunit format adds a
lot of boilerplate function declarations.  Compare that extra
boilerplate to a one line comment.  This is a clarity of source
code argument that I am making.

It may be a little hard to see my point given the current state of
unittest.c.  I could definitely make that much more readable using
the current model.


>>
>> Breaking the tests into separate chunks, each chunk invoked
>> independently as the result of module_test() of each chunk,
>> loses the summary result for the devicetree unittests of
>> how many tests are run and how many passed.  This is the
> 
> We still provide that. Well, we provide a total result of all tests
> run, but they are already grouped by test module, and we could provide
> module level summaries, that would be pretty trivial.

Providing the module level summary (assuming that all of the devicetree
tests were in a single module) would meet this need.


>> only statistic that I need to determine whether the
>> unittests have detected a new fault caused by a specific
>> patch or commit.  I don't need to look at any individual
>> test result unless the overall result reports a failure.
> 
> Yep, we do that too.

Well, when you add the module level summary...


>>
>>
>>> that we would follow up with several other patches like this one and
>>> the subsequent patch, one which would pull out a couple test
>>> functions, as I have done here, and another that splits those
>>> functions up into a bunch of proper test cases.
>>>
>>> I thought that having that many unrelated test cases in a single file
>>> would just be a pain to sort through deal with, review, whatever.
>>
>> Having all the test cases in a single file makes it easier for me to
>> read, understand, modify, and maintain the tests.
> 
> Alright, well that's a much harder thing to make a strong statement
> about. From my experience, I have usually seen one or two *maybe
> three* test suites in a single file, and you have a lot more than that
> in the file right now, but this sounds like a discussion for later
> anyway.

drivers/of/test-common.c is already split out by the patch series.


>>
>>> This is not something I feel particularly strongly about, it is just
>>> pretty atypical from my experience to have so many unrelated test
>>> cases in a single file.
>>>
>>> Maybe you would prefer that I break up the test cases first, and then
>>> we split up the file as appropriate?
>>
>> I prefer that the test cases not be broken up arbitrarily.  There _may_
> 
> I wasn't trying to break them up arbitrarily. I thought I was doing it
> according to a pattern (breaking up the file, that is), but maybe I
> just hadn't looked at enough examples.

This goes back to the kunit model of putting each test into a separate
function that can be a KUNIT_CASE().  That is a model that I do not agree
with for devicetree.


>> be cases where the devicetree unittests are currently not well grouped
>> and may benefit from change, but if so that should be handled independently
>> of any transformation into a KUnit framework.
> 
> I agree. I did this because I wanted to illustrate what I thought real
> world KUnit unit tests should look like (I also wanted to be able to
> show off KUnit test features that help you write these kinds of
> tests); I was not necessarily intending that all the of: unittest
> patches would get merged in with the whole RFC. I was mostly trying to
> create cause for discussion (which it seems like I succeeded at ;-) ).
> 
> So fair enough, I will propose these patches separately and later
> (except of course this one that splits up the file). Do you want the
> initial transformation to the KUnit framework in the main KUnit
> patchset, or do you want that to be done separately? If I recall, Rob
> suggested this as a good initial example that other people could refer
> to, and some people seemed to think that I needed one to help guide
> the discussion and provide direction for early users. I don't
> necessarily think that means the initial real world example needs to
> be a part of the initial patchset though.
> 
> Cheers
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

  reply index

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-28 19:36 [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework Brendan Higgins
2018-11-28 19:36 ` [RFC v3 01/19] kunit: test: add KUnit test runner core Brendan Higgins
2018-11-30  3:14   ` Luis Chamberlain
2018-12-01  1:51     ` Brendan Higgins
2018-12-01  2:57       ` Luis Chamberlain
2018-12-05 13:15     ` Anton Ivanov
2018-12-05 14:45       ` Arnd Bergmann
2018-12-05 14:49         ` Anton Ivanov
2018-11-30  3:28   ` Luis Chamberlain
     [not found]     ` <20181130032802.GG18410-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-01  2:08       ` Brendan Higgins
2018-12-01  3:10         ` Luis Chamberlain
     [not found]           ` <20181201031049.GL28501-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-03 22:47             ` Brendan Higgins
2018-12-01  3:02   ` Luis Chamberlain
2018-11-28 19:36 ` [RFC v3 10/19] kunit: test: add test managed resource tests Brendan Higgins
     [not found] ` <20181128193636.254378-1-brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2018-11-28 19:36   ` [RFC v3 02/19] kunit: test: add test resource management API Brendan Higgins
2018-11-28 19:36   ` [RFC v3 03/19] kunit: test: add string_stream a std::stream like string builder Brendan Higgins
2018-11-30  3:29     ` Luis Chamberlain
2018-12-01  2:14       ` Brendan Higgins
2018-12-01  3:12         ` Luis Chamberlain
2018-12-03 10:55       ` Petr Mladek
2018-12-04  0:35         ` Brendan Higgins
2018-11-28 19:36   ` [RFC v3 04/19] kunit: test: add test_stream a std::stream like logger Brendan Higgins
2018-11-28 19:36   ` [RFC v3 05/19] kunit: test: add the concept of expectations Brendan Higgins
2018-11-28 19:36   ` [RFC v3 06/19] arch: um: enable running kunit from User Mode Linux Brendan Higgins
2018-11-28 21:26     ` Rob Herring
2018-11-30  3:37       ` Luis Chamberlain
2018-11-30 14:05         ` Rob Herring
2018-11-30 18:22           ` Luis Chamberlain
     [not found]             ` <20181130182203.GS18410-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-03 23:22               ` Brendan Higgins
2018-11-30  3:30     ` Luis Chamberlain
2018-11-28 19:36   ` [RFC v3 07/19] kunit: test: add initial tests Brendan Higgins
2018-11-30  3:40     ` Luis Chamberlain
2018-12-03 23:26       ` Brendan Higgins
2018-12-03 23:43         ` Luis Chamberlain
2018-11-28 19:36   ` [RFC v3 08/19] arch: um: add shim to trap to allow installing a fault catcher for tests Brendan Higgins
2018-11-30  3:34     ` Luis Chamberlain
     [not found]       ` <20181130033429.GK18410-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-03 23:34         ` Brendan Higgins
     [not found]           ` <CAFd5g45+MAVaSW8HN9x57Y8Um=TV1Oa=-K8yExPBS-4KjLyciQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-12-03 23:46             ` Luis Chamberlain
     [not found]               ` <20181203234628.GR28501-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-04  0:44                 ` Brendan Higgins
2018-11-30  3:41     ` Luis Chamberlain
2018-12-03 23:37       ` Brendan Higgins
2018-11-28 19:36   ` [RFC v3 09/19] kunit: test: add the concept of assertions Brendan Higgins
2018-11-28 19:36   ` [RFC v3 11/19] kunit: add Python libraries for handing KUnit config and kernel Brendan Higgins
2018-11-29 13:54     ` Kieran Bingham
     [not found]       ` <841cf4ae-501b-05ae-5863-a51010709b67-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
2018-12-03 23:48         ` Brendan Higgins
2018-12-04 20:47           ` Luis Chamberlain
2018-12-06 12:32             ` Kieran Bingham
2018-12-06 15:37               ` Matthew Wilcox
2018-12-07 11:30                 ` Kieran Bingham
2018-12-11 14:09                 ` Petr Mladek
2018-12-11 14:41                   ` Steven Rostedt
2018-12-11 17:01                     ` Anton Ivanov
2019-02-09  0:40                       ` Brendan Higgins
2018-12-07  1:05               ` Luis Chamberlain
2018-12-07 18:35               ` Kent Overstreet
2018-11-30  3:44     ` Luis Chamberlain
2018-12-03 23:50       ` Brendan Higgins
2018-12-04 20:48         ` Luis Chamberlain
2018-11-28 19:36   ` [RFC v3 12/19] kunit: add KUnit wrapper script and simple output parser Brendan Higgins
2018-11-28 19:36   ` [RFC v3 13/19] kunit: improve output from python wrapper Brendan Higgins
2018-11-28 19:36   ` [RFC v3 14/19] Documentation: kunit: add documentation for KUnit Brendan Higgins
2018-11-29 13:56     ` Kieran Bingham
2018-11-30  3:45       ` Luis Chamberlain
     [not found]         ` <20181130034525.GP18410-dAjH6bxAqesAS62YNPtMr3dQhYtBYE6JAL8bYrjMMd8@public.gmane.org>
2018-12-03 23:53           ` Brendan Higgins
2018-12-06 12:16             ` Kieran Bingham
2019-02-09  0:56               ` Brendan Higgins
2019-02-11 12:16                 ` Kieran Bingham
2019-02-12 22:10                   ` Brendan Higgins
2019-02-13 21:55                     ` Kieran Bingham
2019-02-14  0:17                       ` Brendan Higgins
2019-02-14 17:26                         ` Luis Chamberlain
2019-02-14 22:07                           ` Brendan Higgins
2018-11-28 19:36   ` [RFC v3 15/19] MAINTAINERS: add entry for KUnit the unit testing framework Brendan Higgins
2018-11-28 19:36   ` [RFC v3 17/19] of: unittest: migrate tests to run on KUnit Brendan Higgins
2018-11-28 20:56     ` Rob Herring
2018-11-30  0:39       ` Randy Dunlap
     [not found]         ` <18814973-8f0a-4647-a097-fcc3dc0b3cd3-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
2018-12-04  0:13           ` Brendan Higgins
2018-12-04 13:40             ` Rob Herring
     [not found]               ` <CAL_JsqL_PivQbrJFEusdKAy-2EQtKL3OHbmyYSK9bzuTOQegqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-12-05 23:42                 ` Brendan Higgins
2018-12-07  0:41                   ` Rob Herring
2018-12-04  0:08       ` Brendan Higgins
2019-02-13  1:44       ` Brendan Higgins
2019-02-14 20:10         ` Rob Herring
2019-02-14 21:52           ` Brendan Higgins
2019-02-18 22:56         ` Frank Rowand
2019-02-28  0:29           ` Brendan Higgins
2018-12-04 10:56     ` Frank Rowand
2018-11-28 19:36   ` [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest Brendan Higgins
2018-12-04 10:58     ` Frank Rowand
2018-12-05 23:54       ` Brendan Higgins
2019-02-14 23:57         ` Frank Rowand
2019-02-15  0:56           ` Brendan Higgins
2019-02-15  2:05             ` Frank Rowand
2019-02-15 10:56               ` Brendan Higgins
2019-02-18 22:25                 ` Frank Rowand [this message]
2019-02-20 20:44                   ` Frank Rowand
2019-02-20 20:47                     ` Frank Rowand
2019-02-28  3:52                     ` Brendan Higgins
2019-03-22  0:22                       ` Frank Rowand
2019-03-22  1:30                         ` Brendan Higgins
2019-03-22  1:47                           ` Frank Rowand
2019-03-25 22:15                             ` Brendan Higgins
2019-09-20 16:57                           ` Rob Herring
2019-09-21 23:57                             ` Frank Rowand
2019-03-22  1:34                         ` Frank Rowand
2019-03-25 22:18                           ` Brendan Higgins
2018-11-28 19:36   ` [RFC v3 19/19] of: unittest: split up some super large test cases Brendan Higgins
2018-11-28 19:36 ` [RFC v3 16/19] arch: um: make UML unflatten device tree when testing Brendan Higgins
2018-11-28 21:16   ` Rob Herring
     [not found]     ` <CAL_JsqK5cG=QzMBFSZ31_-3ujnxqxv=jj3XYajbRLT7yWYZbfw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-12-04  0:00       ` Brendan Higgins
2018-11-30  3:46   ` Luis Chamberlain
2018-12-04  0:02     ` Brendan Higgins
2018-12-04 10:52 ` [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework Frank Rowand
2018-12-04 11:40 ` Frank Rowand
2018-12-04 13:49   ` Rob Herring
2018-12-05 23:10     ` Brendan Higgins
2019-03-22  0:27       ` Frank Rowand
2019-03-25 22:04         ` Brendan Higgins

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9f7e000-4cac-a35a-3ff9-60130e12ebea@gmail.com \
    --to=frowand.list@gmail.com \
    --cc=Tim.Bird@sony.com \
    --cc=brakmo@fb.com \
    --cc=brendanhiggins@google.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jdike@addtoit.com \
    --cc=joe@perches.com \
    --cc=joel@jms.id.au \
    --cc=julia.lawall@lip6.fr \
    --cc=keescook@google.com \
    --cc=khilman@baylibre.com \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=knut.omang@oracle.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linux-um@lists.infradead.org \
    --cc=mcgrof@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=richard@nod.at \
    --cc=robh@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NVDIMM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvdimm/0 linux-nvdimm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvdimm linux-nvdimm/ https://lore.kernel.org/linux-nvdimm \
		linux-nvdimm@lists.01.org
	public-inbox-index linux-nvdimm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.01.lists.linux-nvdimm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git