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

Tue Sep 5 23:34:32 UTC 2017

On 9/4/2017 1:30 AM, Daniel Vetter wrote:
> 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
>> 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");
>
> Do we really want to remove runtime-dumped information (maybe we could
> tune it down to debug level) with comments that in my experience, no one
> ever reads?

I removed that to avoid duplication as per the previous review comments. 
I do think it's useful to have both.

>
> I just looked again at the igt library docs, and like last year when I've
> done that, we've accumulated sizeable chunks of missing docs and warnings.
> So who's going to maintain this? We can barely keep docs in shape for the
> core libs, how exactly are we going to keep docs in shape for the hundreds
> of testcases we have? Do you have a team of 10+ people working on this?

I think this will be more effective if this becomes a collective 
responsibility, not just an individual team's effort. We should enforce 
regular updates to the documentation during code reviews. We do not need 
to start updating all 300+ tests for now. We can add this form of 
documentation whenever we encounter a test that is hard to follow or 
needs clarification. Same goes for the libraries.

>
> Same about IGT_TEST_DESCRIPTION, not enough people seem to care even for
> that simple one-liner to make it work.
>
> My proposal would be that we first try to get the docs we have into decent
> shape, which means establishing as something everyone takes care of.
> Adding more lofty documentation goals that won't pan out imo just doesn't
> make much sense.

Agree, but if we really want to make this efficient, we can look up the 
API documentation while describing a certain test and update both as 
necessary.

>
> And yes this is from the person who did push for docs almost everywhere.
> It's hard work, and it's a lot of hard work.
> -Daniel
>

I agree. There still seem to be mixed views on even whether we need 
documentation or not.

>> -
>>  	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:
>> +	 * Test creation and use of flink.
>> +	 */
>>  	igt_subtest("basic")
>>  		test_flink(fd);
>> +
>> +	/**
>> +	 * double-flink:
>> +	 * This test validates the ability to create multiple flinks
>> +	 * for the same gem object. They should obtain the same name.
>> +	 */
>>  	igt_subtest("double-flink")
>>  		test_double_flink(fd);
>> +
>> +	/**
>> +	 * bad-flink:
>> +	 * Negative test for invalid flink usage.
>> +	 */
>>  	igt_subtest("bad-flink")
>>  		test_bad_flink(fd);
>> +
>>  	igt_subtest("bad-open")
>>  		test_bad_open(fd);
>> +
>> +	/**
>> +	 * flink-lifetime:
>> +	 * Flink lifetime is limited to that of the gem object it
>> +	 * points to.
>> +	 */
>>  	igt_subtest("flink-lifetime")
>>  		test_flink_lifetime(fd);
>>  }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>