From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752913AbdEPU3G (ORCPT ); Tue, 16 May 2017 16:29:06 -0400 Received: from ms.lwn.net ([45.79.88.28]:45242 "EHLO ms.lwn.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbdEPU3D (ORCPT ); Tue, 16 May 2017 16:29:03 -0400 Date: Tue, 16 May 2017 14:29:01 -0600 From: Jonathan Corbet To: =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= Cc: shuah@kernel.org, linux-kernel@vger.kernel.org, Andy Lutomirski , Kees Cook , Will Drewry , linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v3 0/6] Add kselftest_harness.h Message-ID: <20170516142901.080336b6@lwn.net> In-Reply-To: <099b067d-89ff-35e0-7a64-7c7ccc31cf4d@digikod.net> References: <20170503222612.9187-1-mic@digikod.net> <099b067d-89ff-35e0-7a64-7c7ccc31cf4d@digikod.net> Organization: LWN.net MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 16 May 2017 22:12:39 +0200 Mickaël Salaün wrote: > > I will have to defer to Jon Corbet for Documentation related changes > > and patches. Jon! Could you please review and give me an Ack. > > Jonathan, what do you think about this patches? Sorry, I missed that completely, looking now... > Add metadata to kselftest_harness.h to be able to include the comments > in the Sphinx documentation. > > Changes since v2: > * add reference to the full documentation in the header file (suggested > by Kees Cook) > > Signed-off-by: Mickaël Salaün > Acked-by: Kees Cook > Cc: Andy Lutomirski > Cc: Jonathan Corbet > Cc: Shuah Khan > Cc: Will Drewry > --- > Documentation/dev-tools/kselftest.rst | 58 +++++++ > tools/testing/selftests/kselftest_harness.h | 259 ++++++++++++++++++++-------- > 2 files changed, 245 insertions(+), 72 deletions(-) > > diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst > index 9232ce94612c..92fc7cc3094b 100644 > --- a/Documentation/dev-tools/kselftest.rst > +++ b/Documentation/dev-tools/kselftest.rst > @@ -120,3 +120,61 @@ Contributing new tests (details) > executable which is not tested by default. > TEST_FILES, TEST_GEN_FILES mean it is the file which is used by > test. > + > +Test Harness > +============ > + > +The *kselftest_harness.h* file contains useful helpers to build tests. The > +tests from *tools/testing/selftests/seccomp/seccomp_bpf.c* can be used as > +examples. Minor quibble: in the spirit of minimizing markup, I'd probably not mark up the file names in this way. > + > +Example > +------- > + > +.. code-block:: c > + > + #include "../kselftest_harness.h" > + > + TEST(standalone_test) { > + do_some_stuff; > + EXPECT_GT(10, stuff) { > + stuff_state_t state; > + enumerate_stuff_state(&state); > + TH_LOG("expectation failed with state: %s", state.msg); > + } > + more_stuff; > + ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!"); > + last_stuff; > + EXPECT_EQ(0, last_stuff); > + } > + > + FIXTURE(my_fixture) { > + mytype_t *data; > + int awesomeness_level; > + }; > + FIXTURE_SETUP(my_fixture) { > + self->data = mytype_new(); > + ASSERT_NE(NULL, self->data); > + } > + FIXTURE_TEARDOWN(my_fixture) { > + mytype_free(self->data); > + } > + TEST_F(my_fixture, data_is_good) { > + EXPECT_EQ(1, is_my_data_good(self->data)); > + } > + > + TEST_HARNESS_MAIN So this was moved from the .h file. That's fine if you want to do it that way, but you could have also left it in place and included it with a :doc: directive. Up to you. > + > +Helpers > +------- > + > +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h > + :doc: helpers > + > + > +Operators > +--------- > + > +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h > + :doc: operators > diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h > index 8ba227db46aa..b55be9807af4 100644 > --- a/tools/testing/selftests/kselftest_harness.h > +++ b/tools/testing/selftests/kselftest_harness.h > @@ -4,37 +4,7 @@ > * > * kselftest_harness.h: simple C unit test helper. > * > - * Usage: > - * #include "../kselftest_harness.h" > - * TEST(standalone_test) { > - * do_some_stuff; > - * EXPECT_GT(10, stuff) { > - * stuff_state_t state; > - * enumerate_stuff_state(&state); > - * TH_LOG("expectation failed with state: %s", state.msg); > - * } > - * more_stuff; > - * ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!"); > - * last_stuff; > - * EXPECT_EQ(0, last_stuff); > - * } > - * > - * FIXTURE(my_fixture) { > - * mytype_t *data; > - * int awesomeness_level; > - * }; > - * FIXTURE_SETUP(my_fixture) { > - * self->data = mytype_new(); > - * ASSERT_NE(NULL, self->data); > - * } > - * FIXTURE_TEARDOWN(my_fixture) { > - * mytype_free(self->data); > - * } > - * TEST_F(my_fixture, data_is_good) { > - * EXPECT_EQ(1, is_my_data_good(self->data)); > - * } > - * > - * TEST_HARNESS_MAIN > + * See documentation in Documentation/dev-tools/kselftest.rst > * > * API inspired by code.google.com/p/googletest > */ > @@ -58,7 +28,13 @@ > * Exported APIs > */ > > -/* TEST(name) { implementation } > +/** > + * DOC: helpers > + * > + * .. code-block:: c > + * > + * TEST(name) { implementation } > + * > * Defines a test by name. > * Names must be unique and tests must not be run in parallel. The > * implementation containing block is a function and scoping should be treated It would be nicer to document these as actual functions, rather than using DOC: blocks. It gives you all the standard formatting, index entries, cross-references, etc. A normal kerneldoc header will work with a macro like this. I guess that's my most substantive comment. If you really want to do it this way instead I'll not raise a big fuss, but I would be curious to know what the reason is? Thanks, jon