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

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

[Vinay Belgaumkar]

Added the missing IGT_TEST_DESCRIPTION and some subtest
descriptions.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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.
+	**/
 	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.
+	**/
 	igt_subtest("double-flink")
 		test_double_flink(fd);
+
+	/* bad-flink:
+	Use an invalid flink handle.
+
+	DRM_IOCTL_GEM_FLINK ioctl call should return failure.
+	**/
 	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.
+	**/
 	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.
+	**/
 	igt_subtest("flink-lifetime")
 		test_flink_lifetime(fd);
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests/gem_flink_basic: Add documentation for subtests

[Patchwork]

== Series Details ==

Series: tests/gem_flink_basic: Add documentation for subtests
URL   : https://patchwork.freedesktop.org/series/29499/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
bf45d253648250fc402eee02237366c8882b2053 igt: Add gem_close

with latest DRM-Tip kernel build CI_DRM_3016
428ed27345fb drm-tip: 2017y-08m-29d-17h-43m-11s UTC integration manifest

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> DMESG-WARN (fi-pnv-d510) fdo#101600
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
Test kms_frontbuffer_tracking:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-bdw-5557u)

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:456s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:436s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:365s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:550s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:252s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:534s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:523s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:517s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:438s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:616s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:447s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:422s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:427s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:504s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:472s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:595s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:602s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:537s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:465s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:490s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:443s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:500s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:559s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:404s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_121/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for tests/gem_flink_basic: Add documentation for subtests

[Patchwork]

== Series Details ==

Series: tests/gem_flink_basic: Add documentation for subtests
URL   : https://patchwork.freedesktop.org/series/29499/
State : success

== Summary ==

Test kms_cursor_legacy:
        Subgroup short-flip-before-cursor-atomic-transitions-varying-size:
                skip       -> PASS       (shard-hsw)
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252 +1

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2185 pass:1203 dwarn:0   dfail:0   fail:18  skip:964 time:9426s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_121/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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);
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Belgaumkar, Vinay]

[-- Attachment #1.1: Type: text/plain, Size: 8465 bytes --]



On 8/30/2017 4:12 AM, Michał Winiarski wrote:
> 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@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.

Agreed. Will change it to match that style. The multi-line comments in 
/lib directory actually use this-
/**
  * <name of function>
  */

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

Yes, but we are trying to open the flink in this line of the test-
         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);

I will change it to "open the flink" instead of "access the gem object".

>
> 	/* 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);

Agreed. Also need to add that comment saying the name remains the same 
across the two
applications opening the same gem object.

>
>>   	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");

True, there is some duplication in the comments at this point.

The documentation that I am adding before the subtest call will be 
rolled up by gtkdoc/Sphinx/doxygen, it likely
will not look at the text documentation in the actual code. When we look 
at the rolled up documentation, it
is good to have an idea of when a particular test will pass/fail without 
having to dig into code.

So, yes, there will be some duplication for existing tests. But if we 
start following this method for new tests,
we can have one place to describe what the test does/when does it fail, 
and then expand on anything that is
not very clear in the code itself.

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

Agreed. In this case, it makes more sense to have this clarification in 
the actual test code itself.

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

I am not suggesting we do a C to English translation. The point of this 
patch is to encourage
documenting of subtests so that anyone who is starting out with 
kernel/test development
can get a better idea of what the test is doing and why, before digging 
into the actual code.

Also, do we expect just kernel developers to use these tests? What about 
QA teams who
are usually focused on test execution? This might help them understand a 
little
better what they are using this test for, and give more information to 
debug the test
as well.

It's not possible even for kernel developers to be familiar with every 
feature of the driver.
Having some descriptive tests/comments can make it easier to ramp up to 
a new feature
and reduce debug time in some cases.

We are using gtkdoc today, the generated documentation that we share 
with every release is here -
https://01.org/linuxgraphics/gfx-docs/igt/

If you drill down to the subtest level for any of the tests, all we have 
is the name of the subtest
and a generic one line description of the entire binary. Not sure what 
value that really adds unless
we start describing the purpose of each subtest. There is "some" inline 
documentation in the test
code today, but not nearly enough to understand the purpose of writing 
that test. This was
clearly evidenced by the fact that I missed commenting about the second 
fd being used before
closing the first one. And this is one of the more straightforward tests 
in the IGT suite.

The part about linking this subtest documentation to the tool chain is 
being worked on by Petri,
but this effort is aimed at making our tests a little more easier to 
understand. I agree that we
should focus on the WHAT and WHY for every subtest and explain the HOW 
when needed.

I also agree that over documenting everything is not worthwhile, but 
majority of the IGT tests
today need a little more explanation of their purpose.

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


[-- Attachment #1.2: Type: text/html, Size: 11080 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michał Winiarski]

On Wed, Aug 30, 2017 at 10:49:20AM -0700, Belgaumkar, Vinay wrote:
> 
> 
> On 8/30/2017 4:12 AM, Michał Winiarski wrote:
> > 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@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.
> 
> Agreed. Will change it to match that style. The multi-line comments in /lib
> directory actually use this-
> /**
>  * <name of function>
>  */
> 
> > 
> > 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.
> 
> Yes, but we are trying to open the flink in this line of the test-
>         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);
> 
> I will change it to "open the flink" instead of "access the gem object".
> 
> > 
> > 	/* 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);
> 
> Agreed. Also need to add that comment saying the name remains the same
> across the two
> applications opening the same gem object.
> 
> > 
> > >   	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");
> 
> True, there is some duplication in the comments at this point.
> 
> The documentation that I am adding before the subtest call will be rolled up
> by gtkdoc/Sphinx/doxygen, it likely
> will not look at the text documentation in the actual code. When we look at
> the rolled up documentation, it
> is good to have an idea of when a particular test will pass/fail without
> having to dig into code.
> 
> So, yes, there will be some duplication for existing tests. But if we start
> following this method for new tests,
> we can have one place to describe what the test does/when does it fail, and
> then expand on anything that is
> not very clear in the code itself.
> 
> > 
> > 
> > >   	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.
> 
> Agreed. In this case, it makes more sense to have this clarification in the
> actual test code itself.
> 
> > 
> > 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ł
> 
> I am not suggesting we do a C to English translation. The point of this
> patch is to encourage
> documenting of subtests so that anyone who is starting out with kernel/test
> development
> can get a better idea of what the test is doing and why, before digging into
> the actual code.

I actually tried to show (by splitting the comments and matching to test source)
that it really is a C to English translation.

If by *what* you mean the exact steps that the test is doing (allocate an
object, do things with the object, assert), then I really think that *what* the
test is doing should be clear from reading the code, anything that's not clear
(tricky, can be missed by casual reader) should be explained by plain regular
comments. If you're going to move beyond a simple description, you're inevitably
going to wander into C to English translation territory.

Now, the detailed *why* in case of such basic tests would require a full
description of what flink/open is, how it can be used, (and perhaps also why it
shouldn't be used anymore) - which should really be placed in drm docs rather
than igt. Perhaps we could get away with just saying that the whole binary
contains basic ABI checks for flink/open in this case?

> 
> Also, do we expect just kernel developers to use these tests? What about QA
> teams who
> are usually focused on test execution? This might help them understand a
> little
> better what they are using this test for, and give more information to debug
> the test
> as well.
> 
> It's not possible even for kernel developers to be familiar with every
> feature of the driver.
> Having some descriptive tests/comments can make it easier to ramp up to a
> new feature
> and reduce debug time in some cases.

Agreed, and I'm not against comments. When reviewing code I'm trying to point
out all places that puzzled me/forced a second (or third... or n'th) look
and ask for comments.
But having too detailed comments is actually harmful IMO, both from maintenance
perspective (every time you need to change the test body, the documentation
is suddenly no longer accurate), and (surpisingly) from readability perspective.
English (or any other natural language) is pretty ambiguous (i.e. "gain access
to gem object"), this can cause the reader to misinterpret the documentation and
in consequence make wrong assumptions about how things actually work.

> 
> We are using gtkdoc today, the generated documentation that we share with
> every release is here -
> https://01.org/linuxgraphics/gfx-docs/igt/
> 
> If you drill down to the subtest level for any of the tests, all we have is
> the name of the subtest
> and a generic one line description of the entire binary. Not sure what value
> that really adds unless
> we start describing the purpose of each subtest. There is "some" inline
> documentation in the test
> code today, but not nearly enough to understand the purpose of writing that
> test. This was
> clearly evidenced by the fact that I missed commenting about the second fd
> being used before
> closing the first one. And this is one of the more straightforward tests in
> the IGT suite.

You missed it because when commenting this subtest you didn't go down the C to
English road :) And that's good, the description shouldn't really contain that
info. /* note that we're using fd, not fd2 here */ would probably be enough.

> 
> The part about linking this subtest documentation to the tool chain is being
> worked on by Petri,
> but this effort is aimed at making our tests a little more easier to
> understand. I agree that we
> should focus on the WHAT and WHY for every subtest and explain the HOW when
> needed.
> 
> I also agree that over documenting everything is not worthwhile, but
> majority of the IGT tests
> today need a little more explanation of their purpose.

I agree.

All I'm asking for is to not go too far.

/* Create an object */
ioctl(fd, GEM_CREATE, &create);

Is the definition of going too far.
(even if it's in the description rather than in test body, I think it's even
worse when it's in the description)

-Michał

> 
> > 
> > >   	igt_subtest("flink-lifetime")
> > >   		test_flink_lifetime(fd);
> > >   }
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Belgaumkar, Vinay]

On 8/30/2017 12:39 PM, Michał Winiarski wrote:
> On Wed, Aug 30, 2017 at 10:49:20AM -0700, Belgaumkar, Vinay wrote:
>>
>>
>> On 8/30/2017 4:12 AM, Michał Winiarski wrote:
>>> 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@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.
>>
>> Agreed. Will change it to match that style. The multi-line comments in /lib
>> directory actually use this-
>> /**
>>  * <name of function>
>>  */
>>
>>>
>>> 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.
>>
>> Yes, but we are trying to open the flink in this line of the test-
>>         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);
>>
>> I will change it to "open the flink" instead of "access the gem object".
>>
>>>
>>> 	/* 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);
>>
>> Agreed. Also need to add that comment saying the name remains the same
>> across the two
>> applications opening the same gem object.
>>
>>>
>>>>   	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");
>>
>> True, there is some duplication in the comments at this point.
>>
>> The documentation that I am adding before the subtest call will be rolled up
>> by gtkdoc/Sphinx/doxygen, it likely
>> will not look at the text documentation in the actual code. When we look at
>> the rolled up documentation, it
>> is good to have an idea of when a particular test will pass/fail without
>> having to dig into code.
>>
>> So, yes, there will be some duplication for existing tests. But if we start
>> following this method for new tests,
>> we can have one place to describe what the test does/when does it fail, and
>> then expand on anything that is
>> not very clear in the code itself.
>>
>>>
>>>
>>>>   	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.
>>
>> Agreed. In this case, it makes more sense to have this clarification in the
>> actual test code itself.
>>
>>>
>>> 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ł
>>
>> I am not suggesting we do a C to English translation. The point of this
>> patch is to encourage
>> documenting of subtests so that anyone who is starting out with kernel/test
>> development
>> can get a better idea of what the test is doing and why, before digging into
>> the actual code.
>
> I actually tried to show (by splitting the comments and matching to test source)
> that it really is a C to English translation.
>
> If by *what* you mean the exact steps that the test is doing (allocate an
> object, do things with the object, assert), then I really think that *what* the
> test is doing should be clear from reading the code, anything that's not clear
> (tricky, can be missed by casual reader) should be explained by plain regular
> comments. If you're going to move beyond a simple description, you're inevitably
> going to wander into C to English translation territory.
>
> Now, the detailed *why* in case of such basic tests would require a full
> description of what flink/open is, how it can be used, (and perhaps also why it
> shouldn't be used anymore) - which should really be placed in drm docs rather
> than igt. Perhaps we could get away with just saying that the whole binary
> contains basic ABI checks for flink/open in this case?
>

Hmm, the more I think about this, the WHAT should actually describe what 
aspect of GPU is being tested by this subtest. The WHY should describe 
any applicable use case that is being covered by it. This may not be 
applicable in all cases, since there may be no known use case. Would 
this be a better guideline to follow? We could then describe the HOW in 
the test code if needed (whilst avoiding obvious comments).

>>
>> Also, do we expect just kernel developers to use these tests? What about QA
>> teams who
>> are usually focused on test execution? This might help them understand a
>> little
>> better what they are using this test for, and give more information to debug
>> the test
>> as well.
>>
>> It's not possible even for kernel developers to be familiar with every
>> feature of the driver.
>> Having some descriptive tests/comments can make it easier to ramp up to a
>> new feature
>> and reduce debug time in some cases.
>
> Agreed, and I'm not against comments. When reviewing code I'm trying to point
> out all places that puzzled me/forced a second (or third... or n'th) look
> and ask for comments.
> But having too detailed comments is actually harmful IMO, both from maintenance
> perspective (every time you need to change the test body, the documentation
> is suddenly no longer accurate), and (surpisingly) from readability perspective.
> English (or any other natural language) is pretty ambiguous (i.e. "gain access
> to gem object"), this can cause the reader to misinterpret the documentation and
> in consequence make wrong assumptions about how things actually work.
>

Once we do have documentation, it needs to be maintained along with the 
code. I do not think that is a reason *not* to have documentation. As 
for ambiguity, I agree that we should strive to make it less confusing 
by using accurate descriptions.

>>
>> We are using gtkdoc today, the generated documentation that we share with
>> every release is here -
>> https://01.org/linuxgraphics/gfx-docs/igt/
>>
>> If you drill down to the subtest level for any of the tests, all we have is
>> the name of the subtest
>> and a generic one line description of the entire binary. Not sure what value
>> that really adds unless
>> we start describing the purpose of each subtest. There is "some" inline
>> documentation in the test
>> code today, but not nearly enough to understand the purpose of writing that
>> test. This was
>> clearly evidenced by the fact that I missed commenting about the second fd
>> being used before
>> closing the first one. And this is one of the more straightforward tests in
>> the IGT suite.
>
> You missed it because when commenting this subtest you didn't go down the C to
> English road :) And that's good, the description shouldn't really contain that
> info. /* note that we're using fd, not fd2 here */ would probably be enough.
>
>>
>> The part about linking this subtest documentation to the tool chain is being
>> worked on by Petri,
>> but this effort is aimed at making our tests a little more easier to
>> understand. I agree that we
>> should focus on the WHAT and WHY for every subtest and explain the HOW when
>> needed.
>>
>> I also agree that over documenting everything is not worthwhile, but
>> majority of the IGT tests
>> today need a little more explanation of their purpose.
>
> I agree.
>
> All I'm asking for is to not go too far.
>
> /* Create an object */
> ioctl(fd, GEM_CREATE, &create);
>
> Is the definition of going too far.
> (even if it's in the description rather than in test body, I think it's even
> worse when it's in the description)
>
> -Michał
>

I agree. The right level of documentation will give us the ability to 
trace the test coverage we have for a specific feature as well, without 
having to dig through the actual test code.

>>
>>>
>>>>   	igt_subtest("flink-lifetime")
>>>>   		test_flink_lifetime(fd);
>>>>   }
>>>
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx