Re: [igt-dev] [PATCH i-g-t 01/10] lib: Introduce dynamic subtests

[Petri Latvala <petri.latvala@intel.com>]

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@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!


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev