Bug 99649

Summary: When testlist contains non-existing (removed) test, piglit excepts out
Product: piglit Reporter: Tomi Sarvela <tomi.p.sarvela>
Component: infrastructureAssignee: 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:

Description Tomi Sarvela 2017-02-02 15:00:04 UTC
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
Comment 1 Dylan Baker 2017-02-02 19:29:40 UTC
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.
Comment 2 Tomi Sarvela 2017-02-03 09:16:35 UTC
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.
Comment 3 Tomi Sarvela 2017-02-03 12:00:13 UTC
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.
Comment 4 Arek Hiler 2017-08-15 13:07:45 UTC
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.