[Piglit] [PATCH RFC] framework: Add --skip-missing option

[Arkadiusz Hiler]

On Thu, Jul 27, 2017 at 09:54:13AM -0700, Dylan Baker wrote:
> Quoting Arkadiusz Hiler (2017-07-27 02:24:33)
> > > >  
> > > >  __all__ = [
> > > >      'RegexFilter',
> > > > @@ -314,6 +316,9 @@ class TestProfile(object):
> > > >          if self.forced_test_list:
> > > >              opts = collections.OrderedDict()
> > > >              for n in self.forced_test_list:
> > > > +                if OPTIONS.skip_missing and n not in self.test_list:
> > > > +                    warnings.warn("Test {} missing. Skipping.".format(n))
> > > > +                    continue
> > > >                  opts[n] = self.test_list[n]
> > > >          else:
> > > >              opts = self.test_list  # pylint: disable=redefined-variable-type
> > > 
> > > I agree that we really want this to have an explicit notrun or similar status.
> > > I'm not sure what the right way to do it is. You could probably create some kind
> > > of stub Test instance that just sets a result of NotRun and returns, but that
> > > doesn't feel very elegant.
> > 
> > My thoughts exactly - stub Test for a fixed result.
> > 
> > And I agree that it isn't very pretty, but reworking the code that
> > handles Test instances to have the special case there feels even less
> > elegant.
> > 
> > What do you think about something along the lines of
> > StubedTest(result=NOTRUN), so we can use it for testing as well?
> 
> Generally we use mocking for tests, so I don't think we'll need it for testing,
> but I think it's the best solution we've come up with so far.
> 
> > 
> > 
> > > > diff --git a/framework/programs/run.py b/framework/programs/run.py
> > > > index 6444cfe..bf68f31 100644
> > > > --- a/framework/programs/run.py
> > > > +++ b/framework/programs/run.py
> > > > @@ -208,6 +208,10 @@ def _run_parser(input_):
> > > >                               'isolation. This allows, but does not require, '
> > > >                               'tests to run multiple tests per process. '
> > > >                               'This value can also be set in piglit.conf.')
> > > > +    parser.add_argument("--skip-missing",
> > > > +                        dest="skip_missing",
> > > > +                        action="store_true",
> > > > +                        help="skip on missing tests instead of failing")
> > > >      parser.add_argument("test_profile",
> > > >                          metavar="<Profile path(s)>",
> > > >                          nargs='+',
> > > > @@ -291,6 +295,7 @@ def run(input_):
> > > >      options.OPTIONS.sync = args.sync
> > > >      options.OPTIONS.deqp_mustpass = args.deqp_mustpass
> > > >      options.OPTIONS.process_isolation = args.process_isolation
> > > > +    options.OPTIONS.skip_missing = args.skip_missing
> > > >  
> > > >      # Set the platform to pass to waffle
> > > >      options.OPTIONS.env['PIGLIT_PLATFORM'] = args.platform
> > > > @@ -389,6 +394,7 @@ def resume(input_):
> > > >      options.OPTIONS.sync = results.options['sync']
> > > >      options.OPTIONS.deqp_mustpass = results.options['deqp_mustpass']
> > > >      options.OPTIONS.proces_isolation = results.options['process_isolation']
> > > > +    options.OPTIONS.skip_missing = results.options['skip_missing']
> > > 
> > > As the person who added the options module...
> > > Options was a mis-design from the start. I really don't want to see more code
> > > added to options. If there's a way we could pass this some other way I would
> > > very much prefer that.
> > 
> > It's kind of hard to not add it, as we have to preserve the "skip
> > missing" value in case of a resume.
> > 
> > I didn't like adding the same name in mutiple places, so I understand
> > what you are saying :-)
> > 
> > I can give reworking options a try. Maybe making it
> > serializes/deserializes itself so we won't have to repeat the field
> > names is the way to go?
> 
> I think we could put it in the TestProfile.options attribute, that would give us
> access where we need it, and we could avoid putting it in options.OPTIONS that
> way. We already enforce that forced_test_list is only used with one profile
> anyway so we wouldn't have to worry about that.
> 
> I think you could add it in the same place that we set the forced_test_list, it
> would still need to be added to the metadata, but since it's an option that's
> easy.

Reworked the patch using a dummy test clas. Looks clean and almost
everything work.

Sadly TestProfile.options are not persisted i.e. do not work with
"resume".

Shall I keep it the setting in options.OPTIONS, or do you have other
suggestions?

-- 
Cheers,
Arek