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

[Chris Wilson]

On Fri, Oct 30, 2015 at 09:55:03AM -0200, Paulo Zanoni wrote:
> 2015-10-30 5:56 GMT-02:00 David Weinehall <david.weinehall at linux.intel.com>:
> > On Wed, Oct 28, 2015 at 02:12:15PM -0200, Paulo Zanoni wrote:
> >> 2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall at linux.intel.com>:
> >> > 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 in some cases.
> >> > Until now there's been no unified way of handling this. Remedy
> >> > this by introducing the --all option to igt_core,
> >> > and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> >>
> >> I really think you should explain both your plan and its
> >> implementation in more details here.
> >
> > Well, I don't see how much more there is to explain; the idea is simply
> > that different tests shouldn't implement similar behaviour in different
> > manners (current kms_frontbuffer_tracking uses a command line option,
> > gem_concurrent_blit changes behaviour depending on the file name it's
> > called with).
> 
> What made me write that is that I noticed that now --all is required
> during --list-subtests (thanks for doing this!) but it was not easy to
> notice this in the commit message or in the code. So I was thinking
> something simple, such as a description of how to use the new option
> both when running IGT and when writing subtests:
> 
> "These tests will only appear in --list-subtests if you also specify
> --all. Same for --run-subtest calls. There's this new macro
> igt_subtest_slow_f (maybe igt_subtest_flags_f now?) which should be
> used in case the subtest can be slow/combinatorial."
> 
> >
> >> >
> >> > Signed-off-by: David Weinehall <david.weinehall at linux.intel.com>
> >> > ---
> >> >  lib/igt_core.c                   |  24 +++++
> >> >  lib/igt_core.h                   |   7 ++
> >> >  tests/gem_concurrent_blit.c      |  44 ++++-----
> >> >  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
> >> >  4 files changed, 165 insertions(+), 118 deletions(-)
> >> >
> >> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> >> > index 59127cafe606..6575b9d6bf0d 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;
> >>
> >> The option is called --all, the new subtest macro is _slow and the
> >> variables and enums are called with_slow_combinatorial. Is this
> >> intentional?
> >
> > The option is called --all because "--with-slow-combinatorial" was
> > considered to be too much of a mouthful.  The variables & enums are
> > still retaining these names because they're much more descriptive.
> 
> Ok, let's keep is like this then (in case they don't change with
> _flags suggestion).
> 
> >
> > The macro is called _slow because I wanted to keep it a bit shorter,
> > but I can rename it to _slow_combinatorial if that's preferred.
> 
> I agree _slow_combinatorial is too big, so let's keep it like this.
> Maybe the new version is going to be _flags or something?

igt_subtest_cond_f().

The subtest is included in the test lists if and only if the conditional
experession is true.

And run with igt_subtest_flags.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre