[igt-dev] [PATCH i-g-t 01/10] lib: Introduce dynamic subtests
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Wed Aug 28 10:38:44 UTC 2019
On Wed, Aug 28, 2019 at 01:30:23PM +0300, Petri Latvala wrote:
> On Wed, Aug 28, 2019 at 01:16:10PM +0300, Arkadiusz Hiler wrote:
> > On Fri, Aug 16, 2019 at 12:34:17PM +0300, Petri Latvala wrote:
> > > Dynamic subtests, or subtests of subtests, are individual pieces of
> > > tests that are not statically available all the time.
> > >
> > > A good example of a need for a dynamic subtest is i915 engine listing:
> > > A normal subtest for each engine class ("bsd"), and a dynamic subtest
> > > for each instance ("bsd0", "bsd2", etc). Or a normal subtest for an
> > > operation with a dynamic subtest for every engine there is.
> > >
> > > Another example is a dynamic subtest for pipes: Instead of using
> > > foreach_pipe_static, make one subtest and use foreach_pipe with
> > > dynamic subtests for each pipe.
> > >
> > > v2: Rebase and adapt to igt_describe changes
> > >
> > > Signed-off-by: Petri Latvala <petri.latvala at intel.com>
> > > ---
> > > lib/igt_core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++-----
> > > lib/igt_core.h | 89 ++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 197 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index 1cbb09f9..8b754b9f 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -263,9 +263,12 @@ bool igt_skip_crc_compare;
> > > static bool list_subtests = false;
> > > static bool describe_subtests = false;
> > > static char *run_single_subtest = NULL;
> > > +static char *run_single_dynamic_subtest = NULL;
> > > static bool run_single_subtest_found = false;
> > > static const char *in_subtest = NULL;
> > > +static const char *in_dynamic_subtest = NULL;
> > > static struct timespec subtest_time;
> > > +static struct timespec dynamic_subtest_time;
> > > static clockid_t igt_clock = (clockid_t)-1;
> > > static bool in_fixture = false;
> > > static bool test_with_subtests = false;
> > > @@ -300,6 +303,7 @@ enum {
> > > OPT_LIST_SUBTESTS = 500,
> > > OPT_DESCRIBE_SUBTESTS,
> > > OPT_RUN_SUBTEST,
> > > + OPT_RUN_DYNAMIC_SUBTEST,
> > > OPT_DESCRIPTION,
> > > OPT_DEBUG,
> > > OPT_INTERACTIVE_DEBUG,
> > > @@ -323,6 +327,8 @@ char *igt_frame_dump_path;
> > >
> > > static bool stderr_needs_sentinel = false;
> > >
> > > +static int _igt_dynamic_tests_executed = -1;
> > > +
> > > const char *igt_test_name(void)
> > > {
> > > return command_str;
> > > @@ -354,7 +360,9 @@ static void _igt_log_buffer_dump(void)
> > > {
> > > uint8_t i;
> > >
> > > - if (in_subtest)
> > > + if (in_dynamic_subtest)
> > > + fprintf(stderr, "Dynamic subtest %s failed.\n", in_dynamic_subtest);
> > > + else if (in_subtest)
> > > fprintf(stderr, "Subtest %s failed.\n", in_subtest);
> > > else
> > > fprintf(stderr, "Test %s failed.\n", command_str);
> > > @@ -619,6 +627,7 @@ 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"
> > > " --run-subtest <pattern>\n"
> > > + " --dynamic-subtest <pattern>\n"
> > > " --debug[=log-domain]\n"
> > > " --interactive-debug[=domain]\n"
> > > " --skip-crc-compare\n"
> > > @@ -739,6 +748,7 @@ static int common_init(int *argc, char **argv,
> > > {"list-subtests", no_argument, NULL, OPT_LIST_SUBTESTS},
> > > {"describe", optional_argument, NULL, OPT_DESCRIBE_SUBTESTS},
> > > {"run-subtest", required_argument, NULL, OPT_RUN_SUBTEST},
> > > + {"dynamic-subtest", required_argument, NULL, OPT_RUN_DYNAMIC_SUBTEST},
> >
> > This deviates from --run-subtests but even OPT_RUN_DYNAMIC_SUBTEST has
> > "run" in it. I guess you did it to preserve '--r' shorthand?
>
> Yes :P
>
> > Also interaction betwen --rub-subtest and igt_dynamic_subtest_container
> > is not very obvious.
> >
> > Same for using both --run-subtest and --dynamic-subtest.
> >
> > Maybe it's time to introduce short options?
> >
> > > {"help-description", no_argument, NULL, OPT_DESCRIPTION},
> > > {"debug", optional_argument, NULL, OPT_DEBUG},
> > > {"interactive-debug", optional_argument, NULL, OPT_INTERACTIVE_DEBUG},
> > > @@ -858,6 +868,11 @@ static int common_init(int *argc, char **argv,
> > > if (!list_subtests)
> > > run_single_subtest = strdup(optarg);
> > > break;
> > > + case OPT_RUN_DYNAMIC_SUBTEST:
> > > + assert(optarg);
> > > + if (!list_subtests)
> > > + run_single_dynamic_subtest = strdup(optarg);
> > > + break;
> > > case OPT_DESCRIPTION:
> > > print_test_description();
> > > ret = -1;
> > > @@ -1107,6 +1122,41 @@ bool __igt_run_subtest(const char *subtest_name, const char *file, const int lin
> > > return (in_subtest = subtest_name);
> > > }
> > >
> > > +bool __igt_run_dynamic_subtest(const char *dynamic_subtest_name)
> > > +{
> > > + int i;
> > > +
> > > + assert(in_subtest);
> > > + assert(_igt_dynamic_tests_executed >= 0);
> > > +
> > > + /* check the dynamic_subtest name only contains a-z, A-Z, 0-9, '-' and '_' */
> > > + for (i = 0; dynamic_subtest_name[i] != '\0'; i++)
> > > + if (dynamic_subtest_name[i] != '_' && dynamic_subtest_name[i] != '-'
> > > + && !isalnum(dynamic_subtest_name[i])) {
> > > + igt_critical("Invalid dynamic subtest name \"%s\".\n",
> > > + dynamic_subtest_name);
> > > + igt_exit();
> > > + }
> >
> > Same logic as for normal subtests, could use a helper?
>
> Yes! I'll do that for next revision.
>
>
> >
> > > +
> > > + if (run_single_dynamic_subtest &&
> > > + uwildmat(dynamic_subtest_name, run_single_dynamic_subtest) == 0)
> > > + return false;
> > > +
> > > + igt_kmsg(KMSG_INFO "%s: starting dynamic subtest %s\n",
> > > + command_str, dynamic_subtest_name);
> > > + igt_info("Starting dynamic subtest: %s\n", dynamic_subtest_name);
> > > + fflush(stdout);
> > > + if (stderr_needs_sentinel)
> > > + fprintf(stderr, "Starting dynamic subtest: %s\n", dynamic_subtest_name);
> > > +
> > > + _igt_log_buffer_reset();
> > > +
> > > + _igt_dynamic_tests_executed++;
> > > +
> > > + igt_gettime(&dynamic_subtest_time);
> > > + return (in_dynamic_subtest = dynamic_subtest_name);
> > > +}
> > > +
> > > /**
> > > * igt_subtest_name:
> > > *
> > > @@ -1161,26 +1211,50 @@ void __igt_subtest_group_restore(int save, int desc)
> > > static bool skipped_one = false;
> > > static bool succeeded_one = false;
> > > static bool failed_one = false;
> > > +static bool dynamic_failed_one = false;
> > > +
> > > +bool __igt_enter_dynamic_container(void)
> > > +{
> > > + _igt_dynamic_tests_executed = 0;
> > > + dynamic_failed_one = false;
> > > +
> > > + return true;
> > > +}
> > >
> > > static void exit_subtest(const char *) __attribute__((noreturn));
> > > static void exit_subtest(const char *result)
> > > {
> > > struct timespec now;
> > > + const char *subtest_text = in_dynamic_subtest ? "Dynamic subtest" : "Subtest";
> > > + const char **subtest_name = in_dynamic_subtest ? &in_dynamic_subtest : &in_subtest;
> > > + struct timespec *thentime = in_dynamic_subtest ? &dynamic_subtest_time : &subtest_time;
> > > + jmp_buf *jmptarget = in_dynamic_subtest ? &igt_dynamic_subtest_jmpbuf : &igt_subtest_jmpbuf;
> > >
> > > igt_gettime(&now);
> > > - igt_info("%sSubtest %s: %s (%.3fs)%s\n",
> > > +
> > > + igt_info("%s%s %s: %s (%.3fs)%s\n",
> > > (!__igt_plain_output) ? "\x1b[1m" : "",
> > > - in_subtest, result, igt_time_elapsed(&subtest_time, &now),
> > > + subtest_text, *subtest_name, result,
> > > + igt_time_elapsed(thentime, &now),
> > > (!__igt_plain_output) ? "\x1b[0m" : "");
> > > fflush(stdout);
> > > if (stderr_needs_sentinel)
> > > - fprintf(stderr, "Subtest %s: %s (%.3fs)\n",
> > > - in_subtest, result, igt_time_elapsed(&subtest_time, &now));
> > > + fprintf(stderr, "%s %s: %s (%.3fs)\n",
> > > + subtest_text, *subtest_name,
> > > + result, igt_time_elapsed(thentime, &now));
> > >
> > > igt_terminate_spins();
> > >
> > > - in_subtest = NULL;
> > > - siglongjmp(igt_subtest_jmpbuf, 1);
> > > + if (!in_dynamic_subtest)
> > > + _igt_dynamic_tests_executed = -1;
> > > +
> > > + /* Don't keep the above text in the log, the container would print it again otherwise */
> > > + if (in_dynamic_subtest)
> > > + _igt_log_buffer_reset();
> > > +
> > > + *subtest_name = NULL;
> > > +
> > > + siglongjmp(*jmptarget, 1);
> > > }
> > >
> > > /**
> > > @@ -1211,6 +1285,7 @@ void igt_skip(const char *f, ...)
> > > }
> > >
> > > if (in_subtest) {
> > > + /* Doing the same even if inside a dynamic subtest */
> > > exit_subtest("SKIP");
> > > } else if (test_with_subtests) {
> > > skip_subtests_henceforth = SKIP;
> > > @@ -1267,7 +1342,22 @@ void __igt_skip_check(const char *file, const int line,
> > > */
> > > void igt_success(void)
> > > {
> > > - succeeded_one = true;
> > > + if (in_subtest && !in_dynamic_subtest && _igt_dynamic_tests_executed >= 0) {
> > > + /*
> > > + * We're exiting a dynamic container, yield a result
> > > + * according to the dynamic tests that got
> > > + * executed.
> > > + */
> > > + if (dynamic_failed_one)
> > > + igt_fail(IGT_EXIT_FAILURE);
> > > +
> > > + if (_igt_dynamic_tests_executed == 0)
> > > + igt_skip("No dynamic tests executed.\n");
> > > + }
> > > +
> > > + if (!in_dynamic_subtest)
> > > + succeeded_one = true;
> > > +
> > > if (in_subtest)
> > > exit_subtest("SUCCESS");
> > > }
> > > @@ -1298,10 +1388,17 @@ void igt_fail(int exitcode)
> > > if (in_atexit_handler)
> > > _exit(IGT_EXIT_FAILURE);
> > >
> > > - if (!failed_one)
> > > - igt_exitcode = exitcode;
> > > + if (in_dynamic_subtest) {
> > > + dynamic_failed_one = true;
> > > + } else {
> > > + /* Dynamic subtest containers must not fail explicitly */
> > > + assert(_igt_dynamic_tests_executed < 0 || dynamic_failed_one);
> > > +
> > > + if (!failed_one)
> > > + igt_exitcode = exitcode;
> > >
> > > - failed_one = true;
> > > + failed_one = true;
> > > + }
> > >
> > > /* Silent exit, parent will do the yelling. */
> > > if (test_child)
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > index 177d2431..21289c8e 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -144,6 +144,7 @@ void __igt_fixture_end(void) __attribute__((noreturn));
> > >
> > > /* subtest infrastructure */
> > > jmp_buf igt_subtest_jmpbuf;
> > > +jmp_buf igt_dynamic_subtest_jmpbuf;
> > > typedef int (*igt_opt_handler_t)(int opt, int opt_index, void *data);
> > > #define IGT_OPT_HANDLER_SUCCESS 0
> > > #define IGT_OPT_HANDLER_ERROR -2
> > > @@ -175,6 +176,8 @@ int igt_subtest_init_parse_opts(int *argc, char **argv,
> > > igt_subtest_init_parse_opts(&argc, argv, NULL, NULL, NULL, NULL, NULL);
> > >
> > > bool __igt_run_subtest(const char *subtest_name, const char *file, const int line);
> > > +bool __igt_enter_dynamic_container(void);
> > > +bool __igt_run_dynamic_subtest(const char *dynamic_subtest_name);
> > > #define __igt_tokencat2(x, y) x ## y
> > >
> > > /**
> > > @@ -224,6 +227,92 @@ bool __igt_run_subtest(const char *subtest_name, const char *file, const int lin
> > > #define igt_subtest_f(f...) \
> > > __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> > >
> > > +/**
> > > + * igt_dynamic_subtest_container:
> > > + * @name: name of the subtest
> > > + *
> > > + * This is a magic control flow block which denotes a subtest code
> > > + * block that contains dynamic subtests. The _f variant accepts a
> > > + * printf format string, which is useful for constructing
> > > + * combinatorial tests.
> > > + *
> > > + * Within a dynamic subtest container, explicit failure
> > > + * (e.g. igt_assert) is not allowed, only dynamic subtests themselves
> > > + * will produce test results. igt_skip()/igt_require() is allowed.
> > > + *
> > > + * This is a simpler version of igt_dynamic_subtest_container_f()
> > > + */
> > > +#define igt_dynamic_subtest_container(name) for (; __igt_run_subtest((name), __FILE__, __LINE__) && \
> > > + __igt_enter_dynamic_container() && \
> > > + (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> > > + igt_success())
> > > +#define __igt_dynamic_subtest_container_f(tmp, format...) \
> > > + for (char tmp [256]; \
> > > + snprintf( tmp , sizeof( tmp ), \
> > > + format), \
> > > + __igt_run_subtest(tmp, __FILE__, __LINE__ ) && \
> > > + __igt_enter_dynamic_container() && \
> > > + (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> > > + igt_success())
> > > +
> > > +/**
> > > + * igt_dynamic_subtest_container_f:
> > > + * @...: format string and optional arguments
> > > + *
> > > + * This is a magic control flow block which denotes a subtest code
> > > + * block that contains dynamic subtests. The _f variant accepts a
> > > + * printf format string, which is useful for constructing
> > > + * combinatorial tests.
> > > + *
> > > + * Within a dynamic subtest container, explicit failure
> > > + * (e.g. igt_assert) is not allowed, only dynamic subtests themselves
> > > + * will produce test results. igt_skip()/igt_require() is allowed.
> > > + *
> > > + * Like igt_dynamic_subtest_container(), but also accepts a printf
> > > + * format string instead of a static string.
> > > + */
> >
> > I think we need a bit more of explanation here. It's hard to understand
> > where the asserts go and that this is more like a subtest group and you
> > have to nest igt_dynamic_subtest under it.
> >
> > I think some pseudocode could be usefule, e.g.:
> >
> > igt_main {
> > igt_dynamic_subtest_container("engine-tests") {
> > /* requires ok, no asserts */
> > igt_require(is_awesome(fd));
> >
> > for_each_engine(e) {
> > igt_dynamic_subtest_f("%s", e->name) {
> > /* asserts ok! */
> > igt_fail();
> > }
> > }
> > }
> > }
> >
> > It's also not very clear how this will be reported out.
>
> Yeah, your recent examples of embedding code into documentation with
> the igt_describe code now makes it possible for me to write better
> docs. Noted.
>
> > Or maybe even use "group" instead of "container" here to build the
> > parallel with normal subtest group, the obvious difference being that it
> > takes name.
>
> The pertinent issue is hiding right here in plain view:
>
> Naming.
>
> The main relative for the "container" is not subtest group, but
> subtest. After all, it is an execution point, listed with
> --list-subtests and executed with --run-subtest.
>
> igt_dynamic_subtest_container is the best name I could come up with,
> and it absolutely sucks at conveying the purpose. Deciding on the name
> should be considered to be the most important part of reviewing this
> series, suggestions for that are very welcome!
Ok, this is going to look a bit ridiculous, but read this out:
igt_subtest_with_dynamic_subsubtests("xyz");
- conveys that it's a subtest, so will work with --run-subtest
- makes obvious that it's special because it *contains* something "dynamic"
- makes it clear that subsubtests nest under subtests
igt_dynamic_subsubtest - double sub-, but clarifies nesting
I am not able to come up with anything shorter that this, that would not
be just plain confusing.
--
Cheers,
Arek
More information about the igt-dev
mailing list