On Mon, Sep 04, 2017 at 08:42:58AM +0000, Szwichtenberg, Radoslaw wrote:
> On Fri, 2017-08-11 at 10:20 +0200, Daniel Vetter wrote:
> > On Thu, Aug 10, 2017 at 01:26:47PM +0300, Petri Latvala wrote:
> > > The current documentation for tests is limited to a single string per
> > > test binary. This patch adds support for documenting individual
> > > subtests.
> > > 
> > > The syntax for subtest documentation is:
> > > 
> > >    igt_document_subtest("Frob knobs to see if one of the "
> > >                         "crossbeams will go out of skew on the "
> > >                         "treadle.\n");
> > >    igt_subtest("knob-frobbing-askew")
> > >      test_frob();
> > > 
> > > or with a format string:
> > > 
> > >   for_example_loop(e) {
> > >     igt_document_subtest_f("Frob %s to see if one of the "
> > >                            "crossbeams will go out of skew on the "
> > >                            "treadle.\n", e->readable_name);
> > >     igt_subtest_f("%s-frob-askew", e->name)
> > >       test_frob(e);
> > >   }
> > 
> > I'm not sold on this layout at all. Everywhere else where we do in-line
> > code documentation it is through specially formatted comments. That gives
> > you a lot of freedom for plain-text layout, in combination with some mild
> > markup (gtk-doc for igt and rst for kernel) that result in docs which both
> > look good in the code and look good rendered.
> > 
> > This here essentially restricts you to one-liners, and a one-liner can't
> > really explain what a more complext test does.
> I second that. For many cases one-liner will do - but these more complicated
> cases are really worth the effort when documenting.

I'm definitely not against documenting more involved testcases, e.g.
kms_frontbuffer_tracking. But this RFC here very much suggests a lot more
beaurocracy documenting everything, and not really some in-depth comments
where needed.

> > If we imagine what e.g. Paulo's test documentation in
> > kms_frontbuffer_tracking.c looks like, it'll be bad.
> > 
> > I thought the test documentation that Thomas Wood worked on (no idea about
> > status) would give us the full power and expressiveness of gtkdoc, but for
> > binaries. I think that's what we want.
> > 
> > Then for testcases I think we again want to follow the inline
> > documentation style, perhaps with something like the below:
> > 
> > 
> >     /**
> >      * IGT-Subtest: tests some stuff
> >      *
> >      * Longer explanation of test approach and result evalution.
> >      *
> >      * Maybe over multiple paragraphs with headlines like CAVEATS, or
> >      * explaining hw bugs and stuff
> >      */
> >     igt_subtest("bla-bla-subtest")
> > 
> > 
> > There's also the question of how to associate a given doc entry with more
> > the multiple subtest names that igt_subtest_f can generate, but I guess
> > that should be solveable.
> I don't think its even worth having same text with just single words changed
> generated for every subtest if test name describes the difference (and I guess
> in many cases that is what we have now). It would be good to document such 
> tests
> in groups.

Yes, I don't think it makes much sense to document every subtest. Some are
better documented as groups, many are just plain trivial (e.g.
kms_addfb_basic), and for others it might be better to comment the exact
test approach in-line in the code.
-Daniel

> 
> Thanks,
> Radek
> > 
> > Any, in my opinion documentation has to look pleasing, both in code and
> > rendered, otherwise people will not look at it, not write it and not
> > update it. Or worse, they stick to writing full comments, because that's
> > the only thing that allows them to express what they want to document.
> > 
> > We need something much better imo than this patch series from that pov.
> > 
> > Thanks, Daniel
> > 
> > > The documentation cannot be extracted from just comments, because
> > > associating them with the correct subtest name will then require doing
> > > pattern matching in the documentation generator, for subtests where
> > > the name is generated at runtime using igt_subtest_f.
> > > 
> > > v2: Rebase, change function name in commit message to match code
> > > 
> > > v3: Fix current documentation string tracking, add missing va_end (Vinay)
> > > 
> > > v4: Fix brainfart in __igt_run_subtest
> > > 
> > > Signed-off-by: Petri Latvala <petri.latv...@intel.com>
> > > Acked-by: Leo Li <sunpeng...@amd.com>
> > > Acked-by: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
> > > ---
> > >  lib/igt_aux.c  |   8 ++--
> > >  lib/igt_core.c | 149 
> > > +++++++++++++++++++++++++++++++++++++++++++++---------
> > > ---
> > >  lib/igt_core.h |   6 ++-
> > >  3 files changed, 128 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > > index f428f15..d56f41f 100644
> > > --- a/lib/igt_aux.c
> > > +++ b/lib/igt_aux.c
> > > @@ -311,7 +311,7 @@ static void sig_handler(int i)
> > >   */
> > >  void igt_fork_signal_helper(void)
> > >  {
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           return;
> > >  
> > >   /* We pick SIGCONT as it is a "safe" signal - if we send SIGCONT to
> > > @@ -344,7 +344,7 @@ void igt_fork_signal_helper(void)
> > >   */
> > >  void igt_stop_signal_helper(void)
> > >  {
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           return;
> > >  
> > >   igt_stop_helper(&signal_helper);
> > > @@ -375,7 +375,7 @@ static void __attribute__((noreturn))
> > > shrink_helper_process(int fd, pid_t pid)
> > >   */
> > >  void igt_fork_shrink_helper(int drm_fd)
> > >  {
> > > - assert(!igt_only_list_subtests());
> > > + assert(!igt_only_collect_data());
> > >   igt_require(igt_drop_caches_has(drm_fd, DROP_SHRINK_ALL));
> > >   igt_fork_helper(&shrink_helper)
> > >           shrink_helper_process(drm_fd, getppid());
> > > @@ -473,7 +473,7 @@ void igt_stop_hang_detector(void)
> > >  #else
> > >  void igt_fork_hang_detector(int fd)
> > >  {
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           return;
> > >  }
> > >  
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index c0488e9..f126ef8 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -99,7 +99,7 @@
> > >   *
> > >   * To allow this i-g-t provides #igt_fixture code blocks for setup code
> > > outside
> > >   * of subtests and automatically skips the subtest code blocks 
> > > themselves.
> > > For
> > > - * special cases igt_only_list_subtests() is also provided. For setup 
> > > code
> > > only
> > > + * special cases igt_only_collect_data() is also provided. For setup code
> > > only
> > >   * shared by a group of subtest encapsulate the #igt_fixture block and 
> > > all
> > > the
> > >   * subtestest in a #igt_subtest_group block.
> > >   *
> > > @@ -253,9 +253,9 @@ static unsigned int exit_handler_count;
> > >  const char *igt_interactive_debug;
> > >  
> > >  /* subtests helpers */
> > > -static bool list_subtests = false;
> > > -static char *run_single_subtest = NULL;
> > > -static bool run_single_subtest_found = false;
> > > +static char *single_subtest = NULL;
> > > +static bool single_subtest_found = false;
> > > +static char *current_subtest_documentation = NULL;
> > >  static const char *in_subtest = NULL;
> > >  static struct timespec subtest_time;
> > >  static clockid_t igt_clock = (clockid_t)-1;
> > > @@ -265,6 +265,13 @@ static bool in_atexit_handler = false;
> > >  static enum {
> > >   CONT = 0, SKIP, FAIL
> > >  } skip_subtests_henceforth = CONT;
> > > +static enum {
> > > + EXECUTE_ALL,
> > > + EXECUTE_SINGLE,
> > > + LIST_SUBTESTS,
> > > + DOCUMENT,
> > > + DOCUMENT_SINGLE
> > > +} runmode = EXECUTE_ALL;
> > >  
> > >  bool __igt_plain_output = false;
> > >  
> > > @@ -277,6 +284,8 @@ bool test_child;
> > >  enum {
> > >   OPT_LIST_SUBTESTS,
> > >   OPT_RUN_SUBTEST,
> > > + OPT_DOC_SUBTESTS,
> > > + OPT_DOC_SINGLE_SUBTEST,
> > >   OPT_DESCRIPTION,
> > >   OPT_DEBUG,
> > >   OPT_INTERACTIVE_DEBUG,
> > > @@ -469,7 +478,7 @@ bool __igt_fixture(void)
> > >  {
> > >   assert(!in_fixture);
> > >  
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           return false;
> > >  
> > >   if (skip_subtests_henceforth)
> > > @@ -563,7 +572,7 @@ static void low_mem_killer_disable(bool disable)
> > >  bool igt_exit_called;
> > >  static void common_exit_handler(int sig)
> > >  {
> > > - if (!igt_only_list_subtests()) {
> > > + if (!igt_only_collect_data()) {
> > >           low_mem_killer_disable(false);
> > >           kick_fbcon(true);
> > >   }
> > > @@ -583,7 +592,7 @@ static void print_version(void)
> > >  {
> > >   struct utsname uts;
> > >  
> > > - if (list_subtests)
> > > + if (igt_only_collect_data())
> > >           return;
> > >  
> > >   uname(&uts);
> > > @@ -599,6 +608,8 @@ static void print_usage(const char *help_str, bool
> > > output_on_stderr)
> > >  
> > >   fprintf(f, "Usage: %s [OPTIONS]\n", command_str);
> > >   fprintf(f, "  --list-subtests\n"
> > > +            "  --document-all-subtests\n"
> > > +            "  --document-subtest <pattern>\n"
> > >              "  --run-subtest <pattern>\n"
> > >              "  --debug[=log-domain]\n"
> > >              "  --interactive-debug[=domain]\n"
> > > @@ -710,6 +721,8 @@ static int common_init(int *argc, char **argv,
> > >   static struct option long_options[] = {
> > >           {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> > >           {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> > > +         {"document-all-subtests", 0, 0, OPT_DOC_SUBTESTS},
> > > +         {"document-subtest", 1, 0, OPT_DOC_SINGLE_SUBTEST},
> > >           {"help-description", 0, 0, OPT_DESCRIPTION},
> > >           {"debug", optional_argument, 0, OPT_DEBUG},
> > >           {"interactive-debug", optional_argument, 0,
> > > OPT_INTERACTIVE_DEBUG},
> > > @@ -800,12 +813,24 @@ static int common_init(int *argc, char **argv,
> > >                           igt_log_domain_filter = strdup(optarg);
> > >                   break;
> > >           case OPT_LIST_SUBTESTS:
> > > -                 if (!run_single_subtest)
> > > -                         list_subtests = true;
> > > +                 if (runmode == EXECUTE_ALL)
> > > +                         runmode = LIST_SUBTESTS;
> > >                   break;
> > >           case OPT_RUN_SUBTEST:
> > > -                 if (!list_subtests)
> > > -                         run_single_subtest = strdup(optarg);
> > > +                 if (runmode == EXECUTE_ALL) {
> > > +                         runmode = EXECUTE_SINGLE;
> > > +                         single_subtest = strdup(optarg);
> > > +                 }
> > > +                 break;
> > > +         case OPT_DOC_SUBTESTS:
> > > +                 if (runmode == EXECUTE_ALL)
> > > +                         runmode = DOCUMENT;
> > > +                 break;
> > > +         case OPT_DOC_SINGLE_SUBTEST:
> > > +                 if (runmode == EXECUTE_ALL) {
> > > +                         runmode = DOCUMENT_SINGLE;
> > > +                         single_subtest = strdup(optarg);
> > > +                 }
> > >                   break;
> > >           case OPT_DESCRIPTION:
> > >                   print_test_description();
> > > @@ -837,11 +862,11 @@ out:
> > >   /* exit immediately if this test has no subtests and a subtest or
> > > the
> > >    * list of subtests has been requested */
> > >   if (!test_with_subtests) {
> > > -         if (run_single_subtest) {
> > > -                 igt_warn("Unknown subtest: %s\n",
> > > run_single_subtest);
> > > +         if (runmode == EXECUTE_SINGLE || runmode ==
> > > DOCUMENT_SINGLE) {
> > > +                 igt_warn("Unknown subtest: %s\n", single_subtest);
> > >                   exit(IGT_EXIT_INVALID);
> > >           }
> > > -         if (list_subtests)
> > > +         if (runmode == LIST_SUBTESTS || runmode == DOCUMENT)
> > >                   exit(IGT_EXIT_INVALID);
> > >   }
> > >  
> > > @@ -849,7 +874,7 @@ out:
> > >           /* exit with no error for -h/--help */
> > >           exit(ret == -1 ? 0 : IGT_EXIT_INVALID);
> > >  
> > > - if (!list_subtests) {
> > > + if (!igt_only_collect_data()) {
> > >           kick_fbcon(false);
> > >           kmsg(KERN_INFO "[IGT] %s: executing\n", command_str);
> > >           print_version();
> > > @@ -957,16 +982,38 @@ bool __igt_run_subtest(const char *subtest_name)
> > >                   igt_exit();
> > >           }
> > >  
> > > - if (list_subtests) {
> > > + if (runmode == LIST_SUBTESTS) {
> > >           printf("%s\n", subtest_name);
> > >           return false;
> > >   }
> > >  
> > > - if (run_single_subtest) {
> > > -         if (uwildmat(subtest_name, run_single_subtest) == 0)
> > > + if (runmode == DOCUMENT) {
> > > +         if (current_subtest_documentation) {
> > > +                 printf("%s:\n\n", subtest_name);
> > > +                 printf("%s", current_subtest_documentation);
> > > +                 free(current_subtest_documentation);
> > > +                 current_subtest_documentation = NULL;
> > > +         }
> > > +         return false;
> > > + }
> > > +
> > > + if (runmode == EXECUTE_SINGLE || runmode == DOCUMENT_SINGLE) {
> > > +         bool stop = runmode == DOCUMENT_SINGLE;
> > > +
> > > +         if (uwildmat(subtest_name, single_subtest)) {
> > > +                 single_subtest_found = true;
> > > +
> > > +                 if (runmode == DOCUMENT_SINGLE &&
> > > current_subtest_documentation)
> > > +                         printf("%s",
> > > current_subtest_documentation);
> > > +         } else {
> > > +                 stop = true;
> > > +         }
> > > +
> > > +         free(current_subtest_documentation);
> > > +         current_subtest_documentation = NULL;
> > > +
> > > +         if (stop)
> > >                   return false;
> > > -         else
> > > -                 run_single_subtest_found = true;
> > >   }
> > >  
> > >   if (skip_subtests_henceforth) {
> > > @@ -983,10 +1030,52 @@ bool __igt_run_subtest(const char *subtest_name)
> > >   _igt_log_buffer_reset();
> > >  
> > >   gettime(&subtest_time);
> > > +
> > >   return (in_subtest = subtest_name);
> > >  }
> > >  
> > >  /**
> > > + * igt_document_subtest:
> > > + * @documentation: documentation for the next subtest
> > > + *
> > > + * This function sets the documentation string for the next occurring
> > > subtest.
> > > + */
> > > +void igt_document_subtest(const char *documentation)
> > > +{
> > > + if (runmode == DOCUMENT || runmode == DOCUMENT_SINGLE) {
> > > +         free(current_subtest_documentation);
> > > +         current_subtest_documentation = strdup(documentation);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * igt_document_subtest_f:
> > > + * @documentation: Documentation for the next subtest
> > > + * @...: format string and optional arguments
> > > + *
> > > + * This function sets the documentation string for the next occurring
> > > subtest.
> > > + *
> > > + * Like igt_document_subtest(), but also accepts a printf format
> > > + * string instead of a static string.
> > > + */
> > > +__attribute__((format(printf, 1, 2)))
> > > +void igt_document_subtest_f(const char *documentation, ...)
> > > +{
> > > + int err;
> > > + va_list args;
> > > +
> > > + if (runmode == DOCUMENT || runmode == DOCUMENT_SINGLE) {
> > > +         free(current_subtest_documentation);
> > > +         va_start(args, documentation);
> > > +         err = vasprintf(&current_subtest_documentation,
> > > documentation, args);
> > > +         va_end(args);
> > > +         if (err < 0)
> > > +                 current_subtest_documentation = NULL;
> > > + }
> > > +}
> > > +
> > > +
> > > +/**
> > >   * igt_subtest_name:
> > >   *
> > >   * Returns: The name of the currently executed subtest or NULL if called
> > > from
> > > @@ -998,14 +1087,14 @@ const char *igt_subtest_name(void)
> > >  }
> > >  
> > >  /**
> > > - * igt_only_list_subtests:
> > > + * igt_only_collect_data:
> > >   *
> > > - * Returns: Returns true if only subtest should be listed and any setup
> > > code
> > > + * Returns: Returns true if the running mode is only collecting data and
> > > any setup code
> > >   * must be skipped, false otherwise.
> > >   */
> > > -bool igt_only_list_subtests(void)
> > > +bool igt_only_collect_data(void)
> > >  {
> > > - return list_subtests;
> > > + return runmode != EXECUTE_ALL && runmode != EXECUTE_SINGLE;
> > >  }
> > >  
> > >  void __igt_subtest_group_save(int *save)
> > > @@ -1059,7 +1148,7 @@ void igt_skip(const char *f, ...)
> > >  
> > >   assert(!test_child);
> > >  
> > > - if (!igt_only_list_subtests()) {
> > > + if (!igt_only_collect_data()) {
> > >           va_start(args, f);
> > >           vprintf(f, args);
> > >           va_end(args);
> > > @@ -1443,12 +1532,12 @@ void igt_exit(void)
> > >           g_key_file_free(igt_key_file);
> > >  #endif
> > >  
> > > - if (run_single_subtest && !run_single_subtest_found) {
> > > -         igt_warn("Unknown subtest: %s\n", run_single_subtest);
> > > + if (single_subtest && !single_subtest_found) {
> > > +         igt_warn("Unknown subtest: %s\n", single_subtest);
> > >           exit(IGT_EXIT_INVALID);
> > >   }
> > >  
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           exit(IGT_EXIT_SUCCESS);
> > >  
> > >   /* Calling this without calling one of the above is a failure */
> > > @@ -2012,7 +2101,7 @@ bool igt_run_in_simulation(void)
> > >   */
> > >  void igt_skip_on_simulation(void)
> > >  {
> > > - if (igt_only_list_subtests())
> > > + if (igt_only_collect_data())
> > >           return;
> > >  
> > >   if (!in_fixture && !in_subtest) {
> > > @@ -2087,7 +2176,7 @@ void igt_vlog(const char *domain, enum igt_log_level
> > > level, const char *format,
> > >   program_name = command_str;
> > >  #endif
> > >  
> > > - if (list_subtests && level <= IGT_LOG_WARN)
> > > + if (igt_only_collect_data() && level <= IGT_LOG_WARN)
> > >           return;
> > >  
> > >   if (vasprintf(&line, format, args) == -1)
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > index 1619a9d..275e467 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -197,8 +197,12 @@ bool __igt_run_subtest(const char *subtest_name);
> > >  #define igt_subtest_f(f...) \
> > >   __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> > >  
> > > +void igt_document_subtest(const char *documentation);
> > > +__attribute__((format(printf, 1, 2)))
> > > +void igt_document_subtest_f(const char *documentation, ...);
> > > +
> > >  const char *igt_subtest_name(void);
> > > -bool igt_only_list_subtests(void);
> > > +bool igt_only_collect_data(void);
> > >  
> > >  void __igt_subtest_group_save(int *);
> > >  void __igt_subtest_group_restore(int);
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to