[Intel-gfx] [PATCH i-g-t v2] tests/gem_flink_basic: Add documentation for subtests

[Arkadiusz Hiler]

On Thu, Aug 31, 2017 at 02:33:23PM -0700, Vinay Belgaumkar wrote:
> Added the missing IGT_TEST_DESCRIPTION and some subtest
> descriptions. Trying to establish a method to document

Hey Vinay,

Please add appropriate tag to the subject, as this is clearly an RFC.

> subtests, it should describe the feature being tested
> rather than how. The HOW part can, if needed, be
> described in the test code.
> 
> Documenting subtests will give us a good way to trace
> feature test coverage, and also help a faster ramp
> for understanding the test code.
> 
> v2: Removed duplication, addressed comments, cc'd test author
> 
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Eric Anholt <eric at anholt.net>
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> ---
>  tests/gem_flink_basic.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
> index 26ae7d6..9c8c4c3 100644
> --- a/tests/gem_flink_basic.c
> +++ b/tests/gem_flink_basic.c
> @@ -36,6 +36,8 @@
>  #include <sys/ioctl.h>
>  #include "drm.h"
>  
> +IGT_TEST_DESCRIPTION("Tests for flink - a way to export a gem object by name");
> +
>  static void
>  test_flink(int fd)
>  {
> @@ -44,8 +46,6 @@ test_flink(int fd)
>  	struct drm_gem_open open_struct;
>  	int ret;
>  
> -	igt_info("Testing flink and open.\n");
> -
>  	memset(&create, 0, sizeof(create));
>  	create.size = 16 * 1024;
>  	ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> @@ -69,8 +69,6 @@ test_double_flink(int fd)
>  	struct drm_gem_flink flink2;
>  	int ret;
>  
> -	igt_info("Testing repeated flink.\n");
> -
>  	memset(&create, 0, sizeof(create));
>  	create.size = 16 * 1024;
>  	ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> @@ -92,8 +90,6 @@ test_bad_flink(int fd)
>  	struct drm_gem_flink flink;
>  	int ret;
>  
> -	igt_info("Testing error return on bad flink ioctl.\n");
> -
>  	flink.handle = 0x10101010;
>  	ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
>  	igt_assert(ret == -1 && errno == ENOENT);
> @@ -105,8 +101,6 @@ test_bad_open(int fd)
>  	struct drm_gem_open open_struct;
>  	int ret;
>  
> -	igt_info("Testing error return on bad open ioctl.\n");
> -
>  	open_struct.name = 0x10101010;
>  	ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
>  
> @@ -121,8 +115,6 @@ test_flink_lifetime(int fd)
>  	struct drm_gem_open open_struct;
>  	int ret, fd2;
>  
> -	igt_info("Testing flink lifetime.\n");
> -
>  	fd2 = drm_open_driver(DRIVER_INTEL);
>  
>  	memset(&create, 0, sizeof(create));
> @@ -134,11 +126,13 @@ test_flink_lifetime(int fd)
>  	ret = ioctl(fd2, DRM_IOCTL_GEM_FLINK, &flink);
>  	igt_assert_eq(ret, 0);
>  
> +	/* Open another reference to the gem object */
>  	open_struct.name = flink.name;
>  	ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
>  	igt_assert_eq(ret, 0);
>  	igt_assert(open_struct.handle != 0);
>  
> +	/* Before closing the previous one */
>  	close(fd2);
>  	fd2 = drm_open_driver(DRIVER_INTEL);
>  
> @@ -155,14 +149,36 @@ igt_main
>  	igt_fixture
>  		fd = drm_open_driver(DRIVER_INTEL);
>  
> +	/**
> +	 * basic:

Much better than the previous proposal.

But I do not like having the subtest name twice - in the comment and in
the igt_subtest().

IMO it's better to have a parser that can extract the name from the
following igt_subtest() than having a place where when we have
duplication and we can go out of sync.



I've been following your efforts and I have some question and thoughts
to share.

### Questions

 1. What's the actual problem statement? What are you trying to solve
    here?

 2. Who, in your mind, is the supposed reader of the documentation?
    How's that different form someone who is supposed to look at the
    code directly?

 3. Why are you trying to drive this? What's your motivation?


### My Two (Euro)cents

As Michal stressed, in a reply to the previous revision, we should not
be doing C to English translation. My reasoning:

1. Maintenance hell - if you describe inner workings of tests in too
much detail, you will have two places that you have to update when you
are making even the slightest adjustments.

And people will forget to update the comments, and will receive negative
reviews, and will have to respin the series making the changes again,
this time in the "English" implementation.

To me it's unnecessary rising of the bar for the contributors.

2. Code is enough - I think it's safe to assume that anyone who is
enough technically inclined to understand the English translation of the
code will be able to understand the code itself.

And the code is openly and freely available. So I do not see much use of
embedding it into the documentation.

3. The more manual tasks we have for the tests developers, the less
appealing the project is. If it will get unpleasant, the people will
think twice about contributing - and not to contribute better things,
but whether the chores are worth their time.


### My Expectations

Definitely we should improve IGT documentation and general readability.
But having too much documentation is even wore.

1. Subtest documentation should be as brief as possible and give you good
   intuition on what it is exercising - for actual details people should
   refer the source code.

2. It should not describe the "feature" it is testing, there are other
   places to do that. It should just give enough of a context to be
   understood by someone who has the general idea of the "feature".

3. It should feel like an added value to the developers, not as a
   unnecessary, manual chore.

4. It should feel natural - it should just take a single glance at the
   surrounding code and developer should know what to do - how the
   commenting is done, what style should be assumed. Many people do not
   read guidelines, and the longer the file is, the less likely is that
   someone will read it through. The guidelines should be simple, to the
   point, statements, that are supposed by actual good examples.
   It's also easier to get a good idea what to do when you are are pointed to
   good code in the actual code base instead of some artificial example.

5. Comments inside the tests should be "the last resort", if the code
   could not be rewritten easily in more readable manner. Their main
   purpose is to help people reading code understand non-obvious parts.
   This should be enforced by reviewers - if it takes you too long to
   understand something, comment on that, possibly with suggested
   rewrite/proposition of a comment.


Looking forward to your reply!

-- 
Cheers,
Arek