| Summary: | When testlist contains non-existing (removed) test, piglit excepts out | ||
|---|---|---|---|
| Product: | piglit | Reporter: | Tomi Sarvela <tomi.p.sarvela> |
| Component: | infrastructure | Assignee: | Dylan Baker <baker.dylan.c> |
| Status: | RESOLVED FIXED | QA Contact: | Piglit Mailing List <piglit> |
| Severity: | minor | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
I'm really uncomfortable with this patch. The problem is that this list is both a set of tests to run, and a deterministic order to run them in. When a test that doesn't exist (or never exists) is allowed to just warn then the deterministic ordering is lost. The exception is (as far as I see) before the 'tests' is written out. If you want to see the deterministic, *correct* order the tests have (or would have been without crash) run, the incomplete results have to be read in anyway. So I'm thinking that test regex and testlist are just hints what user would want to run, and thus, user input that might be incorrect. Correct list is in testfiles or result.json. It's always so with regex, why not use same idea with testlist. On this note: I have also patches to keep the correctness of 'tests' order in testrun. These change 'tests' dict to ordereddict on couple of places, adds sorting to os.listdir() and changes internal copying from dict comprehension to OrderedDict comprehension. I was asked to explain my issue a bit better, so here's by pov: Catching the exception is clear. The issue is known and easily hit, and non-catched exception is a bug. Choosing what to do: Continue with Warning - This helps CI: testlist might be maintained locally, and removing test upstream breaks every run after update at the moment - This helps user: warning about missing test, but test anyway what you can - Test order is in meta/tests, just as with regexes Abort with Error - User: makes it clear that testlist can't be run as-is. Force to edit testlist - CI: breaks. Resolution: don't build piglit from upstream automatically - Test order can be read from testlist, in addition to meta/tests One possibility is to choose the behaviour with a commandline option. I wouldn't mind that, either. I don't think testlist are very widely used yet, so choosing the correct default is possible. I'm liking "Continue with Warning", because that's the unix way. Patch adding --ignore-missing which changes piglit behavior so that missing subtests are considered "notrun" is now merged (94f0eface8a876db0a6a0fe0a83c19a0bd1e1eed). The default behavior of exploding with an exception is preserved without the switch. Thanks! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
This trivial patch turns exception into warning if testlist contains non-existing tests. Issue happens easily if testlist is maintained outside framework and subtests (or even tests) are removed. Patch sent to mailing list, but is waiting for moderation. Best regards, Tomi diff --git a/framework/profile.py b/framework/profile.py index 94efd0a..80f1cd3 100644 --- a/framework/profile.py +++ b/framework/profile.py @@ -39,6 +39,7 @@ import multiprocessing import multiprocessing.dummy import os import re +import warnings import six @@ -314,7 +315,8 @@ class TestProfile(object): if self.forced_test_list: opts = collections.OrderedDict() for n in self.forced_test_list: - opts[n] = self.test_list[n] + try: opts[n] = self.test_list[n] + except KeyError: warnings.warn("Test {} missing".format(n), RuntimeWarning) else: opts = self.test_list # pylint: disable=redefined-variable-type