Re: [PATCH v3 0/6] Add kselftest_harness.h

[Jonathan Corbet <corbet@lwn.net>]

On Tue, 16 May 2017 22:12:39 +0200
Mickaël Salaün <mic@digikod.net> 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 <mic@digikod.net>
> Acked-by: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Will Drewry <wad@chromium.org>
> ---
>  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