On Fri, Sep 01, 2017 at 12:55:37PM +0100, Arkadiusz Hiler 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
>
> 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:
I aggree with Arek that there is no need to tell explicitly testcase
name. It is seen from the code.
>
> 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.
>
In my opinion test should be written in a way that person familiar with
IGT and kernel code is able to tell what is going on.
We could add some brief info what/how is tested. But only brief one.
Too much documentation will be confsing and will be a burden to
developers.
Do we really want that?
Regards,
Kasia