[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 "http://llvm.org/PR").

By default this option is globally disabled in lit. It can be globally enabled by passing the flag, and it can be disabled in a test suite's lit.cfg or lit.site.cfg, using the option config.xFailRequirePR. Additionally extra PR prefixes can be specified globally using the --bug-tracker-prefix flag, or per-test suite by setting config.bugTrackerPrefixes.

[beanz]

Looping in @ddunbar and @modocache.

[• ddunbar]

Cute, I like the idea.

Is there a reason to have the command line option, versus just requiring this if the prefixes are defined?

Also, on the syntax, wouldn't it be simpler to say this should look like:

XFAIL: condition, condition # PR or description

[beanz]

I am completely flexible on the syntax. The option os for globally enabling the feature which is disabled by default. There is a RFC thread going on LLVM-dev about whether or not we want to make the accompanying policy change project-wide (pending an audit of existing XFAILs).

[modocache]

This is a great idea, thanks!

I agree with @ddunbar's suggestion on the syntax.

utils/lit/lit/main.py

276

Looks like this will have to be rebased -- these have since been migrated to argparse and add_argument in rL283152.

I agree with @ddunbar's suggestion: specifying --bug-tracker-prefix should imply --xfail-requires-pr, which would eliminate the need for this option.

nit-pick: Modern Python's naming convention for variables is snake_case (https://www.python.org/dev/peps/pep-0008/#function-names), which would make this variable name xfail_requires_pr. One nice part about argparse is that foo-bar is automatically converted to an attribute named foo_bar, so we could eliminate the dest= parameter here. Still, I guess there's little value in migrating this codebase from mixedCase to snake_case, whether incrementally or all at once. But I was wondering if maybe @ddunbar has any thoughts here?

277

nit-pick: If we use "PR" in the help messages for lit, it'd be great to spell out "problem report" in at least one spot in the help messages. I think there's room for confusion, since GitHub users are used to "PR" meaning "pull request".

Related: D25331

282

You can use %(default)s instead of writing out the default value. argparse will replace it with the value specified in the default= parameter.

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,
+                 bugTrackerPrefixes = []):
         # 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.bugTrackerPrefixes = bugTrackerPrefixes
 
     @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 parseIntegratedTestScript(test, require_script=True):
     # Collect the test lines from the script.
     sourcepath = test.getSourcePath()
     script = []
     requires = []
     requires_any = []
     unsupported = []
     keywords = ['RUN:', 'XFAIL:', 'REQUIRES:', 'REQUIRES-ANY:',
                 'UNSUPPORTED:', 'END.']
+
+    bugTrackerRegex = re.compile('^' + '|^'.join(test.config.bugTrackerPrefixes))
     for line_number, command_type, ln in \
             parseIntegratedTestScriptCommands(sourcepath, keywords):
         if command_type == 'RUN':
             # Trim trailing whitespace.
             ln = ln.rstrip()
 
             # Substitute line number expressions
             ln = re.sub('%\(line\)', str(line_number), ln)
             def replace_line_number(match):
                 if match.group(1) == '+':
                     return str(line_number + int(match.group(2)))
                 if match.group(1) == '-':
                     return str(line_number - int(match.group(2)))
             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 test.config.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:

utils/lit/lit/TestingConfig.py

def __init__(self, parent, name, suffixes, test_format,
         self.pipefail = pipefail
         # This list is used by TestRunner.py to restrict running only tests that
         # require one of the features in this list if this list is non-empty.
         # Configurations can set this list to restrict the set of tests to run.
         self.limit_to_features = set(limit_to_features)
         # Whether the suite should be tested early in a given run.
         self.is_early = bool(is_early)
 
+        # Default this to on at the test suite level, but globally it is off.
+        # This allows test suites to opt out on a per-suite basis.
+        self.xFailRequirePR = True
+        self.bugTrackerPrefixes = []
+
     def finish(self, litConfig):
         """finish() - Finish this config object, after loading is complete."""
 
         self.name = str(self.name)
         self.suffixes = set(self.suffixes)
         self.environment = dict(self.environment)
         self.substitutions = list(self.substitutions)
         if self.test_exec_root is not None:
             # FIXME: This should really only be suite in test suite config
             # files. Should we distinguish them?
             self.test_exec_root = str(self.test_exec_root)
         if self.test_source_root is not None:
             # FIXME: This should really only be suite in test suite config
             # files. Should we distinguish them?
             self.test_source_root = str(self.test_source_root)
         self.excludes = set(self.excludes)
+        self.xFailRequirePR = self.xFailRequirePR and litConfig.xFailRequirePR
+        self.bugTrackerPrefixes.extend(litConfig.bugTrackerPrefixes)
 
     @property
     def root(self):
         """root attribute - The root configuration for the test suite."""
         if self.parent is None:
             return self
         else:
             return self.parent.root
 

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
 
litConfig = lit.LitConfig.LitConfig(
         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,
+        bugTrackerPrefixes = opts.bugTrackerPrefixes)
 
     # 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/local-override-disable/lit.cfg

+import lit.formats
+config.name = 'xfail-requires-pr-disabled'
+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'
+config.xFailRequirePR = False

utils/lit/tests/Inputs/xfail-require-pr/local-override-disable/xfail-suite-disabled.txt

+RUN: false
+XFAIL: *

utils/lit/tests/Inputs/xfail-require-pr/local-override-prefix/lit.cfg

+import lit.formats
+config.name = 'xfail-requires-pr-custom'
+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'
+config.bugTrackerPrefixes = ['http://bar.org/']

utils/lit/tests/Inputs/xfail-require-pr/local-override-prefix/xfail-suite-custom-pr.txt

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

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 5)
+# DEFAULT: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 5)
+# DEFAULT: XFAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 5)
+# DEFAULT: XFAIL: xfail-requires-pr-custom :: xfail-suite-custom-pr.txt (4 of 5)
+# DEFAULT: XFAIL: xfail-requires-pr-disabled :: xfail-suite-disabled.txt (5 of 5)
+# DEFAULT: Expected Failures  : 5
+
+
+# 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 5)
+# REQUIRE_PR: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 5)
+# REQUIRE_PR: FAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 5)
+# REQUIRE_PR: XFAIL: xfail-requires-pr-custom :: xfail-suite-custom-pr.txt (4 of 5)
+# REQUIRE_PR: XFAIL: xfail-requires-pr-disabled :: xfail-suite-disabled.txt (5 of 5)
+# REQUIRE_PR: Expected Failures  : 3
+# 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 5)
+# CUSTOM_PR: XFAIL: xfail-requires-pr :: xfail-containing-pr.txt (2 of 5)
+# CUSTOM_PR: FAIL: xfail-requires-pr :: xfail-missing-pr.txt (3 of 5)
+# CUSTOM_PR: XFAIL: xfail-requires-pr-custom :: xfail-suite-custom-pr.txt (4 of 5)
+# CUSTOM_PR: XFAIL: xfail-requires-pr-disabled :: xfail-suite-disabled.txt (5 of 5)
+# CUSTOM_PR: Expected Failures  : 4
+# CUSTOM_PR: Unexpected Failures: 1