linux-kernel - Re: [PATCH v6 01/18] kunit: test: add KUnit test runner core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190705201505.GA19023@42.do-not-panic.com>
Date:   Fri, 5 Jul 2019 20:15:05 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     frowand.list@...il.com, gregkh@...uxfoundation.org,
        jpoimboe@...hat.com, keescook@...gle.com,
        kieran.bingham@...asonboard.com, peterz@...radead.org,
        robh@...nel.org, sboyd@...nel.org, shuah@...nel.org, tytso@....edu,
        yamada.masahiro@...ionext.com, devicetree@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, kunit-dev@...glegroups.com,
        linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-nvdimm@...ts.01.org,
        linux-um@...ts.infradead.org, Alexander.Levin@...rosoft.com,
        Tim.Bird@...y.com, amir73il@...il.com, dan.carpenter@...cle.com,
        daniel@...ll.ch, jdike@...toit.com, joel@....id.au,
        julia.lawall@...6.fr, khilman@...libre.com, knut.omang@...cle.com,
        logang@...tatee.com, mpe@...erman.id.au, pmladek@...e.com,
        rdunlap@...radead.org, richard@....at, rientjes@...gle.com,
        rostedt@...dmis.org, wfg@...ux.intel.com
Subject: Re: [PATCH v6 01/18] kunit: test: add KUnit test runner core

On Wed, Jul 03, 2019 at 05:35:58PM -0700, 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@...gle.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@...tatee.com>

Reviewed-by: Luis Chamberlain <mcgrof@...nel.org>

But a nitpick below, I think that can be fixed later with a follow up
patch.

> +/**
> + * struct kunit - represents a running instance of a test.
> + * @priv: for user to store arbitrary data. Commonly used to pass data created
> + * in the init function (see &struct kunit_suite).
> + *
> + * Used to store information about the current context under which the test is
> + * running. Most of this data is private and should only be accessed indirectly
> + * via public functions; the one exception is @priv which can be used by the
> + * test writer to store arbitrary data.
> + *
> + * A brief note on locking:
> + *
> + * First off, we need to lock because in certain cases a user may want to use an
> + * expectation in a thread other than the thread that the test case is running
> + * in.

This as a prefix to the struct without a lock seems odd. It would be
clearer I think if you'd explain here what locking mechanism we decided
to use and why it suffices today.

> +/**
> + * suite_test() - used to register a &struct kunit_suite with KUnit.

You mean kunit_test_suite()?

> + * @suite: a statically allocated &struct kunit_suite.
> + *
> + * Registers @suite with the test framework. See &struct kunit_suite for more
> + * information.
> + *
> + * NOTE: Currently KUnit tests are all run as late_initcalls; this means that
> + * they cannot test anything where tests must run at a different init phase. One
> + * significant restriction resulting from this is that KUnit cannot reliably
> + * test anything that is initialize in the late_init phase.
                            initialize prior to the late init phase.


That is, this is useless to test things running early.

> + *
> + * TODO(brendanhiggins@...gle.com): Don't run all KUnit tests as late_initcalls.
> + * I have some future work planned to dispatch all KUnit tests from the same
> + * place, and at the very least to do so after everything else is definitely
> + * initialized.

TODOs are odd to be adding to documentation, this is just not common
place practice. The NOTE should suffice for you.

  Luis