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
