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

[Thomas Wood]

On 26 October 2015 at 15:28, David Weinehall
<david.weinehall at linux.intel.com> wrote:
> On Fri, Oct 23, 2015 at 03:55:23PM +0100, Thomas Wood wrote:
>> On 23 October 2015 at 12:42, David Weinehall
>> <david.weinehall at 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?

What is the default set of tests? Each subtest is executed by piglit
using --run-subtest to ensure information can be collected per-subtest
(return code, error messages, dmesg logs, timings, etc.).


>
>>
>> > +                               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.

Test and subtest names for i-g-t are collected and parsed in
piglit/tests/igt.py, which could always be updated include
classification parsing.