* [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-08-10 8:44 [PATCH i-g-t v3 0/5] Subtest documentation Petri Latvala
@ 2017-08-10 8:44 ` Petri Latvala
0 siblings, 0 replies; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 8:44 UTC (permalink / raw)
To: intel-gfx
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);
}
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)
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Acked-by: Leo Li <sunpeng.li@amd.com>
Acked-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
lib/igt_aux.c | 8 ++--
lib/igt_core.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------------
lib/igt_core.h | 6 ++-
3 files changed, 127 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..7da9340 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,37 @@ 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)) {
+ stop = true;
+ single_subtest_found = true;
+
+ if (runmode == DOCUMENT_SINGLE && current_subtest_documentation)
+ printf("%s", current_subtest_documentation);
+ }
+
+ free(current_subtest_documentation);
+ current_subtest_documentation = NULL;
+
+ if (stop)
return false;
- else
- run_single_subtest_found = true;
}
if (skip_subtests_henceforth) {
@@ -983,10 +1029,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 +1086,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 +1147,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 +1531,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 +2100,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 +2175,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH i-g-t v4 0/5] Subtest documentation
@ 2017-08-10 10:26 Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
Changes:
1/5 Fix a brainfart that I thought I tested for. Thanks, CI!
Petri Latvala (5):
Add support for subtest-specific documentation
README: Describe the subtest documentation command line flags
docs: Include subtest documentation
testdisplay: Handle subtest documentation flags
igt_command_line.sh: Test subtest documentation flag handling
README | 5 +
docs/reference/intel-gpu-tools/Makefile.am | 6 ++
lib/igt_aux.c | 8 +-
lib/igt_core.c | 149 +++++++++++++++++++++++------
lib/igt_core.h | 6 +-
tests/igt_command_line.sh | 12 ++-
tests/testdisplay.c | 2 +
7 files changed, 152 insertions(+), 36 deletions(-)
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
@ 2017-08-10 10:26 ` Petri Latvala
2017-08-10 22:27 ` Belgaumkar, Vinay
2017-08-11 8:20 ` Daniel Vetter
2017-08-10 10:26 ` [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags Petri Latvala
` (4 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
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);
}
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@intel.com>
Acked-by: Leo Li <sunpeng.li@amd.com>
Acked-by: Arkadiusz Hiler <arkadiusz.hiler@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(¤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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
@ 2017-08-10 10:26 ` Petri Latvala
2017-08-10 22:59 ` Belgaumkar, Vinay
2017-08-10 10:26 ` [PATCH i-g-t 3/5] docs: Include subtest documentation Petri Latvala
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
README | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/README b/README
index d1ea952..5b6cb00 100644
--- a/README
+++ b/README
@@ -32,6 +32,11 @@ tests/
be run. Some tests have futher options and these are detailed by using
the --help option.
+ Tests with subtests will print a documentation string (if any)
+ with the --document-subtest option. The option
+ --document-all-subtests will print the subtest documentation
+ for all subtests that have it.
+
The test suite can be run using the run-tests.sh script available in
the scripts directory. Piglit is used to run the tests and can either
be installed from your distribution (if available), or can be
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH i-g-t 3/5] docs: Include subtest documentation
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags Petri Latvala
@ 2017-08-10 10:26 ` Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags Petri Latvala
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
A simple and naive format: Double newline denotes paragraph change,
otherwise insert subtest documentation into the generated docs as-is.
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
docs/reference/intel-gpu-tools/Makefile.am | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/docs/reference/intel-gpu-tools/Makefile.am b/docs/reference/intel-gpu-tools/Makefile.am
index ee1e900..2407e37 100644
--- a/docs/reference/intel-gpu-tools/Makefile.am
+++ b/docs/reference/intel-gpu-tools/Makefile.am
@@ -56,6 +56,12 @@ xml/igt_test_programs_%_description.xml: $(TESTLISTS)
for subtest in $$subtest_list; do \
echo "<member>" >> $@; \
echo "$$subtest" | perl -pe 's/\b$(KEYWORDS)\b/<acronym>\1<\/acronym>/g' >> $@; \
+ subtest_doc=`./$$testprog --document-subtest $$subtest`; \
+ if [ -n "$$subtest_doc" ]; then \
+ echo "<para><![CDATA[" >> $@; \
+ echo "$$subtest_doc" | sed ':a;N;$$!ba;s,\n\n,]]></para><para><![CDATA[,g'>> $@; \
+ echo "]]></para>" >> $@; \
+ fi; \
echo "</member>" >> $@; \
done; \
echo "</simplelist>" >> $@; \
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
` (2 preceding siblings ...)
2017-08-10 10:26 ` [PATCH i-g-t 3/5] docs: Include subtest documentation Petri Latvala
@ 2017-08-10 10:26 ` Petri Latvala
2017-08-10 23:08 ` Belgaumkar, Vinay
2017-08-10 10:26 ` [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling Petri Latvala
2017-08-10 10:54 ` ✓ Fi.CI.BAT: success for Subtest documentation (rev2) Patchwork
5 siblings, 1 reply; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
testdisplay has no subtests so hook them to existing subtest handling.
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
tests/testdisplay.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/testdisplay.c b/tests/testdisplay.c
index f2a41fa..85b118f 100644
--- a/tests/testdisplay.c
+++ b/tests/testdisplay.c
@@ -623,6 +623,8 @@ int main(int argc, char **argv)
struct option long_opts[] = {
{"list-subtests", 0, 0, SUBTEST_OPTS},
{"run-subtest", 1, 0, SUBTEST_OPTS},
+ {"document-all-subtests", 0, 0, SUBTEST_OPTS},
+ {"document-subtest", 1, 0, SUBTEST_OPTS},
{"help-description", 0, 0, HELP_DESCRIPTION},
{"help", 0, 0, 'h'},
{"yb", 0, 0, Yb_OPT},
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
` (3 preceding siblings ...)
2017-08-10 10:26 ` [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags Petri Latvala
@ 2017-08-10 10:26 ` Petri Latvala
2017-08-10 23:09 ` Belgaumkar, Vinay
2017-08-10 10:54 ` ✓ Fi.CI.BAT: success for Subtest documentation (rev2) Patchwork
5 siblings, 1 reply; 15+ messages in thread
From: Petri Latvala @ 2017-08-10 10:26 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
tests/igt_command_line.sh | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/igt_command_line.sh b/tests/igt_command_line.sh
index 7f80fc8..b3a794c 100755
--- a/tests/igt_command_line.sh
+++ b/tests/igt_command_line.sh
@@ -102,7 +102,17 @@ for test in $TESTLIST; do
fi
fi
+ # check --document-all-subtests handling
+ echo " Checking subtest documentation..."
+ DOCS=`./$test --document-all-subtests`
+ RET=$?
+
+ if [ $RET -ne 0 -a $RET -ne 79 ]; then
+ fail $test
+ fi
+
# check invalid subtest handling
echo " Checking invalid subtest handling..."
- ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail $test
+ ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail "$test --run-subtest invalid"
+ ./$test --document-subtest invalid-subtest > /dev/null 2>&1 && fail "$test --document-subtest invalid"
done
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for Subtest documentation (rev2)
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
` (4 preceding siblings ...)
2017-08-10 10:26 ` [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling Petri Latvala
@ 2017-08-10 10:54 ` Patchwork
5 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-08-10 10:54 UTC (permalink / raw)
To: Petri Latvala; +Cc: intel-gfx
== Series Details ==
Series: Subtest documentation (rev2)
URL : https://patchwork.freedesktop.org/series/28605/
State : success
== Summary ==
IGT patchset tested on top of latest successful build
c129026622accef6f54c0cfb0dc55e930cfa60b5 igt: add syncobj_basic.
with latest DRM-Tip kernel build CI_DRM_2943
ed9143385873 drm-tip: 2017y-08m-10d-09h-37m-54s UTC integration manifest
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> FAIL (fi-snb-2600) fdo#100215
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:447s
fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:425s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:355s
fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:487s
fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:486s
fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:528s
fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:509s
fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:583s
fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:430s
fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:414s
fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:427s
fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:508s
fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:479s
fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:463s
fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:569s
fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:577s
fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:530s
fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:444s
fi-skl-6700k total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:643s
fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:464s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:424s
fi-skl-x1585l total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:491s
fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:549s
fi-snb-2600 total:279 pass:248 dwarn:0 dfail:0 fail:2 skip:29 time:416s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_52/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
@ 2017-08-10 22:27 ` Belgaumkar, Vinay
2017-08-11 8:20 ` Daniel Vetter
1 sibling, 0 replies; 15+ messages in thread
From: Belgaumkar, Vinay @ 2017-08-10 22:27 UTC (permalink / raw)
To: Petri Latvala, intel-gfx
On 8/10/2017 3:26 AM, 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);
> }
>
> 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@intel.com>
> Acked-by: Leo Li <sunpeng.li@amd.com>
> Acked-by: Arkadiusz Hiler <arkadiusz.hiler@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(¤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)
> }
Document subtest function works fine now, tried it with the latest
patches. Also,
Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>
> /**
> - * 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags
2017-08-10 10:26 ` [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags Petri Latvala
@ 2017-08-10 22:59 ` Belgaumkar, Vinay
0 siblings, 0 replies; 15+ messages in thread
From: Belgaumkar, Vinay @ 2017-08-10 22:59 UTC (permalink / raw)
To: Petri Latvala, intel-gfx
On 8/10/2017 3:26 AM, Petri Latvala wrote:
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> ---
> README | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/README b/README
> index d1ea952..5b6cb00 100644
> --- a/README
> +++ b/README
> @@ -32,6 +32,11 @@ tests/
> be run. Some tests have futher options and these are detailed by using
> the --help option.
>
> + Tests with subtests will print a documentation string (if any)
> + with the --document-subtest option. The option
> + --document-all-subtests will print the subtest documentation
> + for all subtests that have it.
> +
> The test suite can be run using the run-tests.sh script available in
> the scripts directory. Piglit is used to run the tests and can either
> be installed from your distribution (if available), or can be
Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags
2017-08-10 10:26 ` [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags Petri Latvala
@ 2017-08-10 23:08 ` Belgaumkar, Vinay
0 siblings, 0 replies; 15+ messages in thread
From: Belgaumkar, Vinay @ 2017-08-10 23:08 UTC (permalink / raw)
To: Petri Latvala, intel-gfx
On 8/10/2017 3:26 AM, Petri Latvala wrote:
> testdisplay has no subtests so hook them to existing subtest handling.
>
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> ---
> tests/testdisplay.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tests/testdisplay.c b/tests/testdisplay.c
> index f2a41fa..85b118f 100644
> --- a/tests/testdisplay.c
> +++ b/tests/testdisplay.c
> @@ -623,6 +623,8 @@ int main(int argc, char **argv)
> struct option long_opts[] = {
> {"list-subtests", 0, 0, SUBTEST_OPTS},
> {"run-subtest", 1, 0, SUBTEST_OPTS},
> + {"document-all-subtests", 0, 0, SUBTEST_OPTS},
> + {"document-subtest", 1, 0, SUBTEST_OPTS},
> {"help-description", 0, 0, HELP_DESCRIPTION},
> {"help", 0, 0, 'h'},
> {"yb", 0, 0, Yb_OPT},
Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling
2017-08-10 10:26 ` [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling Petri Latvala
@ 2017-08-10 23:09 ` Belgaumkar, Vinay
0 siblings, 0 replies; 15+ messages in thread
From: Belgaumkar, Vinay @ 2017-08-10 23:09 UTC (permalink / raw)
To: Petri Latvala, intel-gfx
On 8/10/2017 3:26 AM, Petri Latvala wrote:
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> ---
> tests/igt_command_line.sh | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/tests/igt_command_line.sh b/tests/igt_command_line.sh
> index 7f80fc8..b3a794c 100755
> --- a/tests/igt_command_line.sh
> +++ b/tests/igt_command_line.sh
> @@ -102,7 +102,17 @@ for test in $TESTLIST; do
> fi
> fi
>
> + # check --document-all-subtests handling
> + echo " Checking subtest documentation..."
> + DOCS=`./$test --document-all-subtests`
> + RET=$?
> +
> + if [ $RET -ne 0 -a $RET -ne 79 ]; then
> + fail $test
> + fi
> +
> # check invalid subtest handling
> echo " Checking invalid subtest handling..."
> - ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail $test
> + ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail "$test --run-subtest invalid"
> + ./$test --document-subtest invalid-subtest > /dev/null 2>&1 && fail "$test --document-subtest invalid"
> done
Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
2017-08-10 22:27 ` Belgaumkar, Vinay
@ 2017-08-11 8:20 ` Daniel Vetter
2017-09-04 8:42 ` Szwichtenberg, Radoslaw
1 sibling, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2017-08-11 8:20 UTC (permalink / raw)
To: Petri Latvala; +Cc: intel-gfx
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.
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.
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@intel.com>
> Acked-by: Leo Li <sunpeng.li@amd.com>
> Acked-by: Arkadiusz Hiler <arkadiusz.hiler@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(¤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@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-08-11 8:20 ` Daniel Vetter
@ 2017-09-04 8:42 ` Szwichtenberg, Radoslaw
2017-09-04 8:54 ` Daniel Vetter
0 siblings, 1 reply; 15+ messages in thread
From: Szwichtenberg, Radoslaw @ 2017-09-04 8:42 UTC (permalink / raw)
To: daniel@ffwll.ch, Latvala, Petri; +Cc: intel-gfx@lists.freedesktop.org
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.
>
> 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.
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@intel.com>
> > Acked-by: Leo Li <sunpeng.li@amd.com>
> > Acked-by: Arkadiusz Hiler <arkadiusz.hiler@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(¤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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH i-g-t 1/5] Add support for subtest-specific documentation
2017-09-04 8:42 ` Szwichtenberg, Radoslaw
@ 2017-09-04 8:54 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2017-09-04 8:54 UTC (permalink / raw)
To: Szwichtenberg, Radoslaw; +Cc: intel-gfx@lists.freedesktop.org
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@intel.com>
> > > Acked-by: Leo Li <sunpeng.li@amd.com>
> > > Acked-by: Arkadiusz Hiler <arkadiusz.hiler@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(¤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@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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-09-04 8:54 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-10 10:26 [PATCH i-g-t v4 0/5] Subtest documentation Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
2017-08-10 22:27 ` Belgaumkar, Vinay
2017-08-11 8:20 ` Daniel Vetter
2017-09-04 8:42 ` Szwichtenberg, Radoslaw
2017-09-04 8:54 ` Daniel Vetter
2017-08-10 10:26 ` [PATCH i-g-t 2/5] README: Describe the subtest documentation command line flags Petri Latvala
2017-08-10 22:59 ` Belgaumkar, Vinay
2017-08-10 10:26 ` [PATCH i-g-t 3/5] docs: Include subtest documentation Petri Latvala
2017-08-10 10:26 ` [PATCH i-g-t 4/5] testdisplay: Handle subtest documentation flags Petri Latvala
2017-08-10 23:08 ` Belgaumkar, Vinay
2017-08-10 10:26 ` [PATCH i-g-t 5/5] igt_command_line.sh: Test subtest documentation flag handling Petri Latvala
2017-08-10 23:09 ` Belgaumkar, Vinay
2017-08-10 10:54 ` ✓ Fi.CI.BAT: success for Subtest documentation (rev2) Patchwork
-- strict thread matches above, loose matches on Subject: below --
2017-08-10 8:44 [PATCH i-g-t v3 0/5] Subtest documentation Petri Latvala
2017-08-10 8:44 ` [PATCH i-g-t 1/5] Add support for subtest-specific documentation Petri Latvala
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox