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

Thomas Wood
2015-10-26T16:28:35Z

On 26 October 2015 at 15:28, David Weinehall
<david.weinehall@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@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.