[LIT] Add option to require PRs for XFAIL directives

[beanz]

Summary

This patch adds a new flag to lit --xfail-requires-pr which causes lit tests to fail if any XFAIL lines do not also reference a PR. To support this functionality this patch changes how XFAIL lines are parsed to allow one entry in the comma separated list to be a PR (starting with the string "PR").

[MatzeB]

This is great idea. Some comments:

• I'd make it require a full URL like http://llvm.org/PRxxxx instead of just PRxxxx so it is easy to get the PR (I am a big fan of Cmd+Click on URL on terminals :)
• Instead of making it a commandline option, I'd rather put the setting into lit.cfg/lit.site.cfg so it will be enforced based on which tests we are running.

[beanz]

@MatzeB, that's a great suggestion. I'll include that in my rework. I also need to add a method of supporting external bug trackers for out-of-tree projects.

[beanz]

• Made PR pattern match LLVM.org URL
• Added support for adding extra PR prefixes
• Added tests and fixed the bugs I found because of them

[MatzeB]

utils/lit/lit/TestRunner.py

830–832

You should use test.config.xFailRequirePR, test.config.bugTrackerRegex after adding some default values to TestingConfig.py. That way these values can be specified in lit.cfg and lit.site.cfg rather than the lit commandline. That will ensure nobody forgets to use those flags and also allows to run test from two testsuites with different bugtrackers in a single lit run.

[probinson]

As a downstream user I would definitely want to put my additional private prefix in a config file. Don't want to have to remember extra command-line options when running tests individually.

[beanz]

I had not intended this patch to be the actual review thread (hence not including llvm-commits). I'm going to adapt to the feedback here and re-post to llvm-commits since the llvm-dev thread was mostly favorable.

test/CMakeLists.txt

list(APPEND LLVM_TEST_DEPENDS
     Kaleidoscope-Ch3
     Kaleidoscope-Ch4
     Kaleidoscope-Ch5
     Kaleidoscope-Ch6
     Kaleidoscope-Ch7
     )
 endif()
 
+option(LLVM_XFAILS_REQUIRE_PRS "Require PRs for all tests with XFAIL annotations" Off)
+if(LLVM_XFAILS_REQUIRE_PRS)
+  list(APPEND LLVM_LIT_ARGS "--xfail-requires-pr")
+endif()
+
 add_custom_target(llvm-test-depends DEPENDS ${LLVM_TEST_DEPENDS})
 set_target_properties(llvm-test-depends PROPERTIES FOLDER "Tests")
 
 add_lit_testsuite(check-llvm "Running the LLVM regression tests"
   ${CMAKE_CURRENT_BINARY_DIR}
   PARAMS llvm_site_config=${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
          llvm_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
   DEPENDS ${LLVM_TEST_DEPENDS}

test/ExecutionEngine/MCJIT/stubs-sm-pic.ll

 ; RUN: %lli -disable-lazy-compilation=false -relocation-model=pic -code-model=small %s
-; XFAIL: mips-, mipsel-, i686, i386, aarch64, arm
+; XFAIL: mips-, mipsel-, i686, i386, aarch64, arm, http://llvm.org/PR12345
 
 define i32 @main() nounwind {
 entry:
   call void @lazily_compiled_address_is_consistent()
   ret i32 0
 }
 
 ; Test PR3043: @test should have the same address before and after

utils/lit/lit/LitConfig.py

class LitConfig(object):
     easily.
     """
 
     def __init__(self, progname, path, quiet,
                  useValgrind, valgrindLeakCheck, valgrindArgs,
                  noExecute, debug, isWindows,
                  params, config_prefix = None,
                  maxIndividualTestTime = 0,
-                 maxFailures = None):
+                 maxFailures = None,
+                 xFailRequirePR = False,
+                 bugTrackerRegex = None):
         # The name of the test runner.
         self.progname = progname
         # The items to add to the PATH environment variable.
         self.path = [str(p) for p in path]
         self.quiet = bool(quiet)
         self.useValgrind = bool(useValgrind)
         self.valgrindLeakCheck = bool(valgrindLeakCheck)
         self.valgrindUserArgs = list(valgrindArgs)
def __init__(self, progname, path, quiet,
                 self.valgrindArgs.append('--leak-check=full')
             else:
                 # The default is 'summary'.
                 self.valgrindArgs.append('--leak-check=no')
             self.valgrindArgs.extend(self.valgrindUserArgs)
 
         self.maxIndividualTestTime = maxIndividualTestTime
         self.maxFailures = maxFailures
+        self.xFailRequirePR = xFailRequirePR
+        self.bugTrackerRegex = bugTrackerRegex
 
     @property
     def maxIndividualTestTime(self):
         """
             Interface for getting maximum time to spend executing
             a single test
         """
         return self._maxIndividualTestTime

utils/lit/lit/Test.py

 PASS        = ResultCode('PASS', False)
 FLAKYPASS   = ResultCode('FLAKYPASS', False)
 XFAIL       = ResultCode('XFAIL', False)
 FAIL        = ResultCode('FAIL', True)
 XPASS       = ResultCode('XPASS', True)
 UNRESOLVED  = ResultCode('UNRESOLVED', True)
 UNSUPPORTED = ResultCode('UNSUPPORTED', False)
 TIMEOUT     = ResultCode('TIMEOUT', True)
+UNXFAIL     = ResultCode('UNXFAIL', False)
 
 # Test metric values.
 
 class MetricValue(object):
     def format(self):
         """
         format() -> str
 
class Test:
     def __init__(self, suite, path_in_suite, config, file_path = None):
         self.suite = suite
         self.path_in_suite = path_in_suite
         self.config = config
         self.file_path = file_path
         # A list of conditions under which this test is expected to fail. These
         # can optionally be provided by test format handlers, and will be
         # honored when the test result is supplied.
-        self.xfails = []
+        self.xfails = {}
         # The test result, once complete.
         self.result = None
 
     def setResult(self, result):
         if self.result is not None:
             raise ArgumentError("test result already set")
         if not isinstance(result, Result):
             raise ArgumentError("unexpected result type")
 
         self.result = result
 
         # Apply the XFAIL handling to resolve the result exit code.
         if self.isExpectedToFail():
             if self.result.code == PASS:
                 self.result.code = XPASS
             elif self.result.code == FAIL:
                 self.result.code = XFAIL
+            elif self.result.code == UNXFAIL:
+                self.result.code = FAIL
         
     def getFullName(self):
         return self.suite.config.name + ' :: ' + '/'.join(self.path_in_suite)
 
     def getFilePath(self):
         if self.file_path:
             return self.file_path
         return self.getSourcePath()
def isExpectedToFail(self):
 
         Check whether this test is expected to fail in the current
         configuration. This check relies on the test xfails property which by
         some test formats may not be computed until the test has first been
         executed.
         """
 
         # Check if any of the xfails match an available feature or the target.
-        for item in self.xfails:
+        for feature, pr in self.xfails.iteritems():
             # If this is the wildcard, it always fails.
-            if item == '*':
+            if feature == '*':
                 return True
 
             # If this is an exact match for one of the features, it fails.
-            if item in self.config.available_features:
+            if feature in self.config.available_features:
                 return True
 
             # If this is a part of the target triple, it fails.
-            if item and item in self.suite.config.target_triple:
+            if feature and feature in self.suite.config.target_triple:
                 return True
 
         return False
 
     def isEarlyTest(self):
         """
         isEarlyTest() -> bool
 

utils/lit/lit/TestRunner.py

def processLine(ln):
             ln = re.sub(a, b, ln)
 
         # Strip the trailing newline and any extra whitespace.
         return ln.strip()
     # Note Python 3 map() gives an iterator rather than a list so explicitly
     # convert to list before returning.
     return list(map(processLine, script))
 
-def parseIntegratedTestScript(test, require_script=True):
+def parseIntegratedTestScript(test, require_script=True, xFailRequirePR=False, bugTrackerRegex=None):
     """parseIntegratedTestScript - Scan an LLVM/Clang style integrated test
     script and extract the lines to 'RUN' as well as 'XFAIL' and 'REQUIRES'
     and 'UNSUPPORTED' information. If 'require_script' is False an empty script
     may be returned. This can be used for test formats where the actual script
     is optional or ignored.
     """
     # Collect the test lines from the script.
     sourcepath = test.getSourcePath()
for line_number, command_type, ln in \
             ln = re.sub('%\(line *([\+-]) *(\d+)\)', replace_line_number, ln)
 
             # Collapse lines with trailing '\\'.
             if script and script[-1][-1] == '\\':
                 script[-1] = script[-1][:-1] + ln
             else:
                 script.append(ln)
         elif command_type == 'XFAIL':
-            test.xfails.extend([s.strip() for s in ln.split(',')])
+            xfail_prs = [s.strip() for s in ln.split(',') if bugTrackerRegex.match(s.strip())]
+            if len(xfail_prs) > 1:
+                raise ValueError("XFAIL command cannot have more than one PR, "
+                                 "use multiple xfail commands.")
+            xfail_pr = None
+            if(len(xfail_prs) > 0):
+                xfail_pr = xfail_prs[0]
+            xfail_features = [s.strip() for s in ln.split(',') if not bugTrackerRegex.match(s.strip())]
+            for feature in xfail_features:
+                test.xfails[feature] = xfail_pr
         elif command_type == 'REQUIRES':
             requires.extend([s.strip() for s in ln.split(',')])
         elif command_type == 'REQUIRES-ANY':
             requires_any.extend([s.strip() for s in ln.split(',')])
         elif command_type == 'UNSUPPORTED':
             unsupported.extend([s.strip() for s in ln.split(',')])
         elif command_type == 'END':
             # END commands are only honored if the rest of the line is empty.
if test.config.limit_to_features:
         # Check that we have one of the limit_to_features features in requires.
         limit_to_features_tests = [f for f in test.config.limit_to_features
                                    if f in requires]
         if not limit_to_features_tests:
             msg = ', '.join(test.config.limit_to_features)
             return lit.Test.Result(Test.UNSUPPORTED,
                  "Test requires one of the limit_to_features features %s" % msg)
 
+    if xFailRequirePR:
+        for feature, pr in test.xfails.iteritems():
+            if pr is None:
+                return lit.Test.Result(Test.UNXFAIL, "XFAIL command missing PR")
+
     return script
 
 def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
     # Create the output directory if it does not already exist.
     lit.util.mkdir_p(os.path.dirname(tmpBase))
 
     execdir = os.path.dirname(test.getExecPath())
     if useExternalSh:
def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
     return lit.Test.Result(status, output)
 
 
 def executeShTest(test, litConfig, useExternalSh,
                   extra_substitutions=[]):
     if test.config.unsupported:
         return (Test.UNSUPPORTED, 'Test is unsupported')
 
-    script = parseIntegratedTestScript(test)
+    script = parseIntegratedTestScript(test,
+                                       xFailRequirePR=litConfig.xFailRequirePR,
+                                       bugTrackerRegex=litConfig.bugTrackerRegex)
     if isinstance(script, lit.Test.Result):
         return script
     if litConfig.noExecute:
         return lit.Test.Result(Test.PASS)
 
     tmpDir, tmpBase = getTempPaths(test)
     substitutions = list(extra_substitutions)
     substitutions += getDefaultSubstitutions(test, tmpDir, tmpBase,

utils/lit/lit/main.py

group.add_option("", "--show-tests", dest="showTests",
                       help="Show all discovered tests",
                       action="store_true", default=False)
     group.add_option("", "--use-processes", dest="useProcesses",
                       help="Run tests in parallel with processes (not threads)",
                       action="store_true", default=True)
     group.add_option("", "--use-threads", dest="useProcesses",
                       help="Run tests in parallel with threads (not processes)",
                       action="store_false", default=True)
+    group.add_option("", "--xfail-requires-pr", dest="xFailRequirePR",
+                      help="Require PRs for XFAIL tests",
+                      action="store_true", default=False)
+    parser.add_option("", "--bug-tracker-prefix", dest="bugTrackerPrefixes",
+                      metavar="",
+                      help=("Prefixes for specifying bugs in XFAIL directives "
+                            "(defaults to include http://llvm.org/PR)"),
+                      type=str, action="append", default=['http://llvm.org/PR'])
     parser.add_option_group(group)
 
     (opts, args) = parser.parse_args()
 
     if opts.show_version:
         print("lit %s" % (lit.__version__,))
         return
 
# threads by default there.
     # Decide what the requested maximum indvidual test time should be
     if opts.maxIndividualTestTime != None:
         maxIndividualTestTime = opts.maxIndividualTestTime
     else:
         # Default is zero
         maxIndividualTestTime = 0
 
     isWindows = platform.system() == 'Windows'
+    bugTrackerRegex = re.compile('^' + '|^'.join(opts.bugTrackerPrefixes))
 
     # Create the global config object.
     litConfig = lit.LitConfig.LitConfig(
         progname = os.path.basename(sys.argv[0]),
         path = opts.path,
         quiet = opts.quiet,
         useValgrind = opts.useValgrind,
         valgrindLeakCheck = opts.valgrindLeakCheck,
         valgrindArgs = opts.valgrindArgs,
         noExecute = opts.noExecute,
         debug = opts.debug,
         isWindows = isWindows,
         params = userParams,
         config_prefix = opts.configPrefix,
         maxIndividualTestTime = maxIndividualTestTime,
-        maxFailures = opts.maxFailures)
+        maxFailures = opts.maxFailures,
+        xFailRequirePR = opts.xFailRequirePR,
+        bugTrackerRegex = bugTrackerRegex)
 
     # Perform test discovery.
     run = lit.run.Run(litConfig,
                       lit.discovery.find_tests_for_inputs(litConfig, inputs))
 
     # After test discovery the configuration might have changed
     # the maxIndividualTestTime. If we explicitly set this on the
     # command line then override what was set in the test configuration

utils/lit/tests/Inputs/xfail-require-pr/lit.cfg

+import lit.formats
+config.name = 'xfail-requires-pr'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+config.target_triple = 'x86_64-unknown-unknown'

utils/lit/tests/Inputs/xfail-require-pr/xfail-containing-custom-pr.txt

+RUN: false
+XFAIL: *, http://foo.org/PR123

utils/lit/tests/Inputs/xfail-require-pr/xfail-containing-pr.txt

+RUN: false
+XFAIL: *, http://llvm.org/PR123

utils/lit/tests/Inputs/xfail-require-pr/xfail-missing-pr.txt

+RUN: false
+XFAIL: *

utils/lit/tests/xfail-require-pr.py

+# RUN: %{lit} -j 1 -v %{inputs}/xfail-require-pr | FileCheck --check-prefix=DEFAULT %s
+# DEFAULT: XFAIL: xfail-requires-pr :: xfail-containing-custom-pr.txt (1 of 3)
+# DEFAULT: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 3)
+# DEFAULT: XFAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 3)
+# DEFAULT: Expected Failures  : 3
+
+
+# RUN: not %{lit} -j 1 -v %{inputs}/xfail-require-pr --xfail-requires-pr | FileCheck --check-prefix=REQUIRE_PR %s
+# REQUIRE_PR: FAIL: xfail-requires-pr :: xfail-containing-custom-pr.txt (1 of 3)
+# REQUIRE_PR: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 3)
+# REQUIRE_PR: FAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 3)
+# REQUIRE_PR: Expected Failures  : 1
+# REQUIRE_PR: Unexpected Failures: 2
+
+# RUN: not %{lit} -j 1 -v %{inputs}/xfail-require-pr --xfail-requires-pr --bug-tracker-prefix http://foo.org/ | FileCheck --check-prefix=CUSTOM_PR %s
+# CUSTOM_PR: XFAIL: xfail-requires-pr :: xfail-containing-custom-pr.txt (1 of 3)
+# CUSTOM_PR: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 3)
+# CUSTOM_PR: FAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 3)
+# CUSTOM_PR: Expected Failures  : 2
+# CUSTOM_PR: Unexpected Failures: 1