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

[MickaÃl SalaÃn]

On 16/05/2017 22:29, Jonathan Corbet wrote:
> On Tue, 16 May 2017 22:12:39 +0200
> MickaÃl SalaÃn <mic@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
>> Cc: Jonathan Corbet <corbet@xxxxxxx>
>> Cc: Shuah Khan <shuah@xxxxxxxxxx>
>> Cc: Will Drewry <wad@xxxxxxxxxxxx>
>> ---
>>  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.

OK

> 
>> +
>> +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.

Keeping it in the .h file means not benefiting from the C syntax
highlighting (even with ".. code-block:: c"). It looks like a bug, though.

> 
>> +
>> +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 can do that but a macro defined as "#define TEST TEST_API(TEST)"
doesn't get an argument with kerneldoc. I guess it is better than a DOC
block, though.

> 
> 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
>

Attachment: signature.asc
Description: OpenPGP digital signature