|
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@xxxxxxxxx> --- 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); Agreed. Also need to add that comment saying the name remains the same across the two applications opening the same gem object.
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.
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.
|
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
