[Piglit] [PATCH RFC] framework: Add --skip-missing option
Dylan Baker
dylan at pnwbakers.com
Fri Aug 11 00:00:49 UTC 2017
Quoting Arkadiusz Hiler (2017-07-31 05:15:26)
> 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".
So, after my filesystem died, I forgot to reinstall my mail sending program, so
I received but not of my mail went out. I have a mail in my box from replying to
this mail on August 1st, but it never got sent, so I'll try again, this is the
verbatim message I tried to send then:
Sorry for the late reply, my filesystem ate itself friday and I'm just now back
up and running.
You will need to manually add it to the _create_metadata function to be stored
in the metadata.json and be available to be restored in resume like we do with
other options like dmesg and monitoring. In fact, dmesg and monitoring are doing
exactly what I'd really like to see us do with this option, being put into
profile.options and saved into the metadata.
Dylan
Sorry again for the very, very late reply,
Dylan
>
> Shall I keep it the setting in options.OPTIONS, or do you have other
> suggestions?
>
> --
> Cheers,
> Arek
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20170810/c6519ab0/attachment.sig>
More information about the Piglit
mailing list