Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642804 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/6] Add kselftest_harness.h Mickaël Salaün <mic@digikod.net> - 2017-05-16 22:20 +0200
    Re: [PATCH v3 0/6] Add kselftest_harness.h Jonathan Corbet <corbet@lwn.net> - 2017-05-16 22:30 +0200
      Re: [PATCH v3 0/6] Add kselftest_harness.h Mickaël Salaün <mic@digikod.net> - 2017-05-16 23:50 +0200

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

[Multipart message — attachments visible in raw view] — view raw

On 04/05/2017 15:58, Shuah Khan wrote:
> On 05/03/2017 04:26 PM, Mickaël Salaün wrote:
>> Hi,
>>
>> This third patch series make the seccomp/test_harness.h more generally
>> available [1] and update the kselftest documentation with the Sphinx format. It
>> also improve the Makefile of seccomp tests to take into account any
>> kselftest_harness.h update.
> 
> Thanks for the test harness as well as updating kselftest documentation
> to rst format. Awesome. I will merge these into linux-kselftest after
> the 4.12-rc1 comes out.
> 
> 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?

> 
>>
>> [1] https://lkml.kernel.org/r/CAGXu5j+8CVz8vL51DRYXqOY=xc3zuKFf=PTENe88XYHzFYidUQ@mail.gmail.com
>>
>> Regards,
>>
>> Mickaël Salaün (6):
>>   selftests: Make test_harness.h more generally available
>>   selftests: Cosmetic renames in kselftest_harness.h
>>   selftests/seccomp: Force rebuild according to dependencies
>>   Documentation/dev-tools: Add kselftest
>>   Documentation/dev-tools: Use reStructuredText markups for kselftest
>>   Documentation/dev-tools: Add kselftest_harness documentation
>>
>>  Documentation/00-INDEX                             |   2 -
>>  Documentation/dev-tools/index.rst                  |   1 +
>>  .../{kselftest.txt => dev-tools/kselftest.rst}     | 125 +++++++---
>>  MAINTAINERS                                        |   1 +
>>  .../test_harness.h => kselftest_harness.h}         | 270 +++++++++++++++------
>>  tools/testing/selftests/seccomp/Makefile           |   2 +
>>  tools/testing/selftests/seccomp/seccomp_bpf.c      |   2 +-
>>  7 files changed, 296 insertions(+), 107 deletions(-)
>>  rename Documentation/{kselftest.txt => dev-tools/kselftest.rst} (50%)
>>  rename tools/testing/selftests/{seccomp/test_harness.h => kselftest_harness.h} (80%)
>>
> 
> thanks,
> -- Shuah
> 

[toc] | [next] | [standalone]

#1642806

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

[toc] | [prev] | [next] | [standalone]

#1642851

[Multipart message — attachments visible in raw view] — view raw

On 16/05/2017 22:29, Jonathan Corbet wrote:
> 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.

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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web