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

[Michał Winiarski]

On Tue, Aug 29, 2017 at 02:25:19PM -0700, Vinay Belgaumkar wrote:
> Added the missing IGT_TEST_DESCRIPTION and some subtest
> descriptions.
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> ---
>  tests/gem_flink_basic.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
> index 26ae7d6..8761e0d 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)
>  {
> @@ -155,14 +157,48 @@ igt_main
>  	igt_fixture
>  		fd = drm_open_driver(DRIVER_INTEL);
>  
> +	/* basic:
> +	This subtest creates a gem object, and then creates
> +	a flink. It tests that we can gain access to the gem
> +	object using the flink name.
> +
> +	Test fails if flink creation/open fails.
> +	**/

Please use kernel coding style.
This is not the format we're using for multiline comments.

/*
 *
 */
^^^ This is the format we're using.

And on the documentation itself, let's take a quote from the kernel coding
style:
"Comments are good, but there is also a danger of over-commenting.  NEVER
try to explain HOW your code works in a comment: it's much better to
write the code so that the **working** is obvious, and it's a waste of
time to explain badly written code."

Now, let's try to match the tests with the comments:
	/* This subtest creates a gem object */ 
	ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
	igt_assert_eq(ret, 0);

	/* and then creates a flink */
	flink.handle = create.handle;
	ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
	igt_assert_eq(ret, 0);

	/* It tests that we can gain access to the gem object using the flink
	 * name
	 */
Well... not really, we're not accessing the object in any way.

	/* Test fails if flink creation/open fails. */
	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);

>  	igt_subtest("basic")
>  		test_flink(fd);
> +
> +	/* double-flink:
> +	This test checks if it is possible to create 2 flinks
> +	for the same gem object.
> +
> +	Test fails if 2 flink objects cannot be created.
> +	**/

	/* This test checks if it is possible to create 2 flinks for the same
	 * gem object
	 */
	
	flink.handle = create.handle;
	ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
	igt_assert_eq(ret, 0);

	flink2.handle = create.handle;
	ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink2);
	igt_assert_eq(ret, 0);

	/* Test fails if 2 flink objects cannot be created. */
Well - this is handled by the asserts above.
You ignored this assumption in your description for some reason though:
	igt_assert(flink2.name == flink.name);

>  	igt_subtest("double-flink")
>  		test_double_flink(fd);
> +
> +	/* bad-flink:
> +	Use an invalid flink handle.
> +
> +	DRM_IOCTL_GEM_FLINK ioctl call should return failure.
> +	**/

	ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
	igt_assert(ret == -1 && errno == ENOENT);

There is also an igt_info message:
	igt_info("Testing error return on bad flink ioctl.\n");


>  	igt_subtest("bad-flink")
>  		test_bad_flink(fd);
> +
> +	/* bad-open:
> +	Try to use an invalid flink name.
> +
> +	DRM_IOCTL_GEM_FLINK ioctl call should return failure.
> +	**/

	open_struct.name = 0x10101010;
	ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);

	igt_assert(ret == -1 && errno == ENOENT);

Same as for bad flink:
	igt_info("Testing error return on bad open ioctl.\n");

>  	igt_subtest("bad-open")
>  		test_bad_open(fd);
> +
> +	/* flink-lifetime:
> +	Check if a flink name can be used even after the drm
> +	fd used to create it is closed.
> +
> +	Flink name should remain valid until the gem object
> +	it points to has not been freed.
> +	**/

That's better, however...
Why wasn't the object freed when we closed the drm fd (fd2) used to create it?
(hint, it wasn't freed because we're doing OPEN using a different fd before
closing fd2, and that changes the lifetime of an object since we're bumping the
refcount this way, which perhaps could use a comment, not in the description
but in the testcase itself).
As for a one-line description, perhaps something more general would work better?
Check if a flink name is valid for the whole duration of underlying gem object
lifetime.

Overall - do you believe, that 1:1 from C to English translation is not a
perfect example of "over-commenting"? Do we really need to take an approach
where we're documenting even the simple ABI checks (e.g. invalid usage - error)?
What value does such documentation have and for whom? I would expect developers
to be able to consume C, are we trying to explain things for non-developers?

-Michał

>  	igt_subtest("flink-lifetime")
>  		test_flink_lifetime(fd);
>  }