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

[David Weinehall <david.weinehall@linux.intel.com>]

On Mon, Oct 26, 2015 at 04:28:15PM +0000, Thomas Wood wrote:
> 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.).

The default set would be the tests that are run when running
gem_concurrent_blit or kms_frontbuffer_tracking without specifying
--all.

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

It would probably make sense to do this, but considering that I neither
have proper python-fu nor know enough about the various test cases to
classify them properly, I think that's orthogonal to this changeset.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx