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.latvala@xxxxxxxxx> > > > Acked-by: Leo Li <sunpeng.li@xxxxxxx> > > > Acked-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> > > > --- > > > 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(¤t_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@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx