Re: [PATCH i-g-t 2/3] Unify handling of slow/combinatorial tests

David Weinehall
2015-10-26T15:28:39Z

On Fri, Oct 23, 2015 at 03:55:23PM +0100, Thomas Wood wrote:
> On 23 October 2015 at 12:42, David Weinehall
> <david.weinehall@linux.intel.com> wrote:
> > Some tests should not be run by default, due to their slow,
> > and sometimes superfluous, nature.
> >
> > We still want to be able to run these tests though in some cases.
> > Until now there's been no unified way of handling this. Remedy
> > this by introducing the --with-slow-combinatorial option to
> > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> > ---
> > lib/igt_core.c | 19 ++++++
> > lib/igt_core.h | 1 +
> > tests/gem_concurrent_blit.c | 40 ++++++++----
> > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------
> > 4 files changed, 142 insertions(+), 53 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 59127cafe606..ba40ce0e0ead 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -216,6 +216,7 @@ const char *igt_interactive_debug;
> >
> > /* subtests helpers */
> > static bool list_subtests = false;
> > +static bool with_slow_combinatorial = false;
> > static char *run_single_subtest = NULL;
> > static bool run_single_subtest_found = false;
> > static const char *in_subtest = NULL;
> > @@ -235,6 +236,7 @@ bool test_child;
> >
> > enum {
> > OPT_LIST_SUBTESTS,
> > + OPT_WITH_SLOW_COMBINATORIAL,
> > OPT_RUN_SUBTEST,
> > OPT_DESCRIPTION,
> > OPT_DEBUG,
> > @@ -478,6 +480,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"
> > + " --with-slow-combinatorial\n"
> > " --run-subtest <pattern>\n"
> > " --debug[=log-domain]\n"
> > " --interactive-debug[=domain]\n"
> > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv,
> > int c, option_index = 0, i, x;
> > static struct option long_options[] = {
> > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL},
> > {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> > {"help-description", 0, 0, OPT_DESCRIPTION},
> > {"debug", optional_argument, 0, OPT_DEBUG},
> > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv,
> > if (!run_single_subtest)
> > list_subtests = true;
> > break;
> > + case OPT_WITH_SLOW_COMBINATORIAL:
> > + if (!run_single_subtest)
>
> This will cause piglit (and therefore QA) to unconditionally run all
> tests marked as slow, since it runs subtests individually.

Why doesn't piglit run the default set of tests instead?

>
> > + with_slow_combinatorial = true;
> > + break;
> > case OPT_RUN_SUBTEST:
> > if (!list_subtests)
> > run_single_subtest = strdup(optarg);
> > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void)
> > igt_require(!igt_run_in_simulation());
> > }
> >
> > +/**
> > + * igt_slow_combinatorial:
> > + *
> > + * This is used to define subtests that should only be listed/run
> > + * when the "--with-slow-combinatorial" has been specified
>
> This isn't quite correct, as the subtests that use
> igt_slow_combinatorial will still always be listed.

Yeah, I agree that the comment is incorrect; it should say "be run",
or alternatively the code altered to not list them unless "--all"
is passed.

> > + */
> > +void igt_slow_combinatorial(void)
> > +{
> > + igt_skip_on(!with_slow_combinatorial);
>
> Although it is convenient to just skip the tests when the
> --with-slow-combinatorial flag is passed, it may be useful to be able
> to classify the subtests before they are run, so that they are
> filtered out from the test list entirely. An approach that can do this
> might also be used to mark tests as being part of the basic acceptance
> tests, so that they can be marked as such without relying on the
> naming convention.

If the list is how piglit gets its list of tests, doign classification
won't be feasible, since only "testname" or "testname (SKIP)" are
valid, TTBOMK.