[lit] Add builtin support for flaky tests in lit

[ldionne]

Summary

This commit adds a new keyword in lit called ALLOW_FAILURES. This keyword
takes a single integer as an argument, and it allows the test to fail that
number of times before it first succeeds.

This work attempts to make the existing test_retry_attempts more flexible
by allowing by-test customization, as well as eliminate libc++'s FLAKY_TEST
custom logic.

[ldionne]

Note that an alternative design would be to drop the integer from ALLOW_FAILURES: and make it a tag keyword only. Then, config.test_retry_attempts could be applied only on the tests that are marked with that keyword. I actually prefer this approach, however it is a behavior change for existing users of config.test_retry_attempts.

Also, I'm open to changing ALLOW_FAILURES to something else, like ALLOW_RETRIES. Suggestions welcome.

Finally, note that this doesn't handle XFAIL tests that would succeed repeatedly. This is actually an existing condition of config.test_retry_attempts, and the root cause is that we translate test results higher up in the call chain, near _execute in worker.py. Unfortunately, handling ALLOW_FAILURES at that point involves parsing the test several times, which messes up things like test.xfails. Since this is a pre-existing condition, I did not try to fix it cause it would have required a probably-breaking refactoring.

[rnk]

In D76288#1926688, @ldionne wrote:

Note that an alternative design would be to drop the integer from ALLOW_FAILURES: and make it a tag keyword only. Then, config.test_retry_attempts could be applied only on the tests that are marked with that keyword. I actually prefer this approach, however it is a behavior change for existing users of config.test_retry_attempts.

I guess I prefer this approach for now. So far, the big users of lit (clang and llvm) have few flaky tests, so lit has had no support for retries or deflaking. With your keyword approach, hopefully flaky tests will not proliferate too quickly.

Also, I'm open to changing ALLOW_FAILURES to something else, like ALLOW_RETRIES. Suggestions welcome.

I guess I like ALLOW_RETRIES: N the best. My other idea was RETRY_LIMIT.

Finally, note that this doesn't handle XFAIL tests that would succeed repeatedly. This is actually an existing condition of config.test_retry_attempts, and the root cause is that we translate test results higher up in the call chain, near _execute in worker.py. Unfortunately, handling ALLOW_FAILURES at that point involves parsing the test several times, which messes up things like test.xfails. Since this is a pre-existing condition, I did not try to fix it cause it would have required a probably-breaking refactoring.

I don't think we need to put any effort into making this work. Flaky failing tests should be disabled instead.

Basically, looks good, but I'd suggest a different tag. Thanks!

[ldionne]

In D76288#1926868, @rnk wrote:

In D76288#1926688, @ldionne wrote:

[...]

I don't think we need to put any effort into making this work. Flaky failing tests should be disabled instead.

Basically, looks good, but I'd suggest a different tag. Thanks!

Great, thanks! I'll change the keyword. Two questions:

1. Is this set of reviewers fine for lit? I'm not familiar with that component.
2. Is there documentation for the ShTest format? I could add something to https://llvm.org/docs/TestingGuide.html, or maybe we're better off keeping this feature on the down low?

[ldionne]

Rename the keyword to ALLOW_RETRIES

[rnk]

lgtm

As far as code ownership goes, I think @yln has been actively working on lit recently, and might be a good reviewer. I think @delcypher has moved on to other projects, but I can't speak for them.

llvm/utils/lit/lit/TestRunner.py

1508

I'm curious, do you find this logic of having one try and then adding N retries reasonable or confusing? I wasn't sure about it when I wrote it. Should the term "retry" be exclusive from the first try?

llvm/utils/lit/tests/allow-retries.py

4

I see, cleaning up the counter file is taken care of at a higher level.

[ldionne]

llvm/utils/lit/lit/TestRunner.py

1508

I think it is reasonable. For me, a *retry* is something you do after the first try. So the total number of attempts is retries + 1 -- it makes sense that way in my head.

llvm/utils/lit/tests/allow-retries.py

4

I'm cleaning up *before* I run the test to make sure the counter file does not already exist, which is the case when running the tests over and over again.

[yln]

This looks like a reasonable way to enable per-test retries. (I feel that test_retry_attempts was tacked on as an afterthought.)
Thanks for adding tests, and also thanks to Reid for giving good feedback.

LGTM, with one quick ask to add 2 more tests to document the behavior/intereaction with config.test_retry_attempts:

• test_retry_attempts (without ALLOW_RETRIES) should still be respected (this test should have been there in the first place).
• ALLOW_RETRIES overwrites test_retry_attempts, when both are specified.

[ldionne]

Add tests as requested

[ldionne]

In D76288#1927738, @yln wrote:

LGTM, with one quick ask to add 2 more tests to document the behavior/intereaction with config.test_retry_attempts:

• test_retry_attempts (without ALLOW_RETRIES) should still be respected (this test should have been there in the first place).
• ALLOW_RETRIES overwrites test_retry_attempts, when both are specified.

Thanks for the feedback, I implemented the tests. I'll commit now.

llvm/utils/lit/lit/Test.py

def __init__(self, suite, path_in_suite, config, file_path = None):
         # FIXME should target triple parts count here too?
         self.requires = []
 
         # A list of conditions that prevent execution of the test.
         # Each condition is a boolean expression of features and target
         # triple parts. All of them must be False for the test to run.
         self.unsupported = []
 
+        # An optional number of retries allowed before the test finally succeeds.
+        # The test is run at most once plus the number of retries specified here.
+        self.allowed_retries = getattr(config, 'test_retry_attempts', 0)
+
         # The test result, once complete.
         self.result = None
 
     def setResult(self, result):
         assert self.result is None, "result already set"
         assert isinstance(result, Result), "unexpected result type"
         self.result = result
 

llvm/utils/lit/lit/TestRunner.py

class ParserKind(object):
     An enumeration representing the style of an integrated test keyword or
     command.
 
     TAG: A keyword taking no value. Ex 'END.'
     COMMAND: A keyword taking a list of shell commands. Ex 'RUN:'
     LIST: A keyword taking a comma-separated list of values.
     BOOLEAN_EXPR: A keyword taking a comma-separated list of 
         boolean expressions. Ex 'XFAIL:'
+    INTEGER: A keyword taking a single integer. Ex 'ALLOW_RETRIES:'
     CUSTOM: A keyword with custom parsing semantics.
     """
     TAG = 0
     COMMAND = 1
     LIST = 2
     BOOLEAN_EXPR = 3
-    CUSTOM = 4
+    INTEGER = 4
+    CUSTOM = 5
 
     @staticmethod
     def allowedKeywordSuffixes(value):
         return { ParserKind.TAG:          ['.'],
                  ParserKind.COMMAND:      [':'],
                  ParserKind.LIST:         [':'],
                  ParserKind.BOOLEAN_EXPR: [':'],
+                 ParserKind.INTEGER:      [':'],
                  ParserKind.CUSTOM:       [':', '.']
                } [value]
 
     @staticmethod
     def str(value):
         return { ParserKind.TAG:          'TAG',
                  ParserKind.COMMAND:      'COMMAND',
                  ParserKind.LIST:         'LIST',
                  ParserKind.BOOLEAN_EXPR: 'BOOLEAN_EXPR',
+                 ParserKind.INTEGER:      'INTEGER',
                  ParserKind.CUSTOM:       'CUSTOM'
                } [value]
 
 
 class IntegratedTestKeywordParser(object):
     """A parser for LLVM/Clang style integrated test scripts.
 
     keyword: The keyword to parse for. It must end in either '.' or ':'.
def __init__(self, keyword, kind, parser=None, initial_value=None):
         if kind == ParserKind.COMMAND:
             self.parser = lambda line_number, line, output: \
                                  self._handleCommand(line_number, line, output,
                                                      self.keyword)
         elif kind == ParserKind.LIST:
             self.parser = self._handleList
         elif kind == ParserKind.BOOLEAN_EXPR:
             self.parser = self._handleBooleanExpr
+        elif kind == ParserKind.INTEGER:
+            self.parser = self._handleSingleInteger
         elif kind == ParserKind.TAG:
             self.parser = self._handleTag
         elif kind == ParserKind.CUSTOM:
             if parser is None:
                 raise ValueError("ParserKind.CUSTOM requires a custom parser")
             self.parser = parser
         else:
             raise ValueError("Unknown kind '%s'" % kind)
class IntegratedTestKeywordParser(object):
     def _handleList(line_number, line, output):
         """A parser for LIST type keywords"""
         if output is None:
             output = []
         output.extend([s.strip() for s in line.split(',')])
         return output
 
     @staticmethod
+    def _handleSingleInteger(line_number, line, output):
+        """A parser for INTEGER type keywords"""
+        if output is None:
+            output = []
+        try:
+            n = int(line)
+        except ValueError:
+            raise ValueError("INTEGER parser requires the input to be an integer (got {})".format(line))
+        output.append(n)
+        return output
+
+    @staticmethod
     def _handleBooleanExpr(line_number, line, output):
         """A parser for BOOLEAN_EXPR type keywords"""
         parts = [s.strip() for s in line.split(',') if s.strip() != '']
         if output and output[-1][-1] == '\\':
             output[-1] = output[-1][:-1] + parts[0]
             del parts[0]
         if output is None:
             output = []
         output.extend(parts)
         # Evaluate each expression to verify syntax.
         # We don't want any results, just the raised ValueError.
         for s in output:
             if s != '*' and not s.endswith('\\'):
                 BooleanExpression.evaluate(s, [])
         return output
 
 def parseIntegratedTestScript(test, additional_parsers=[],
                               require_script=True):
     """parseIntegratedTestScript - Scan an LLVM/Clang style integrated test
-    script and extract the lines to 'RUN' as well as 'XFAIL' and 'REQUIRES'
-    and 'UNSUPPORTED' information.
+    script and extract the lines to 'RUN' as well as 'XFAIL', 'REQUIRES',
+    'UNSUPPORTED' and 'ALLOW_RETRIES' information.
 
     If additional parsers are specified then the test is also scanned for the
     keywords they specify and all matches are passed to the custom parser.
 
     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.
     """
 
     # Install the built-in keyword parsers.
     script = []
     builtin_parsers = [
         IntegratedTestKeywordParser('RUN:', ParserKind.COMMAND,
                                     initial_value=script),
         IntegratedTestKeywordParser('XFAIL:', ParserKind.BOOLEAN_EXPR,
                                     initial_value=test.xfails),
         IntegratedTestKeywordParser('REQUIRES:', ParserKind.BOOLEAN_EXPR,
                                     initial_value=test.requires),
         IntegratedTestKeywordParser('UNSUPPORTED:', ParserKind.BOOLEAN_EXPR,
                                     initial_value=test.unsupported),
+        IntegratedTestKeywordParser('ALLOW_RETRIES:', ParserKind.INTEGER),
         IntegratedTestKeywordParser('END.', ParserKind.TAG)
     ]
     keyword_parsers = {p.keyword: p for p in builtin_parsers}
     
     # Install user-defined additional parsers.
     for parser in additional_parsers:
         if not isinstance(parser, IntegratedTestKeywordParser):
             raise ValueError('additional parser must be an instance of '
def parseIntegratedTestScript(test, additional_parsers=[],
     unsupported_features = test.getUnsupportedFeatures()
     if unsupported_features:
         msg = ', '.join(unsupported_features)
         return lit.Test.Result(
             Test.UNSUPPORTED,
             "Test does not support the following features "
             "and/or targets: %s" % msg)
 
+    # Handle ALLOW_RETRIES:
+    allowed_retries = keyword_parsers['ALLOW_RETRIES:'].getValue()
+    if allowed_retries:
+        if len(allowed_retries) > 1:
+            return lit.Test.Result(Test.UNRESOLVED,
+                                   "Test has more than one ALLOW_RETRIES lines")
+        test.allowed_retries = allowed_retries[0]
+
     # Enforce limit_to_features.
     if not test.isWithinFeatureLimits():
         msg = ', '.join(test.config.limit_to_features)
         return lit.Test.Result(Test.UNSUPPORTED,
                                "Test does not require any of the features "
                                "specified in limit_to_features: %s" % msg)
 
     return script
if litConfig.noExecute:
         return lit.Test.Result(Test.PASS)
 
     tmpDir, tmpBase = getTempPaths(test)
     substitutions = list(extra_substitutions)
     substitutions += getDefaultSubstitutions(test, tmpDir, tmpBase,
                                              normalize_slashes=useExternalSh)
     script = applySubstitutions(script, substitutions)
 
-    # Re-run failed tests up to test_retry_attempts times.
-    attempts = 1
-    if hasattr(test.config, 'test_retry_attempts'):
-        attempts += test.config.test_retry_attempts
+    # Re-run failed tests up to test.allowed_retries times.
+    attempts = test.allowed_retries + 1
     for i in range(attempts):
         res = _runShTest(test, litConfig, useExternalSh, script, tmpBase)
         if res.code != Test.FAIL:
             break
     # If we had to run the test more than once, count it as a flaky pass. These
     # will be printed separately in the test summary.
     if i > 0 and res.code == Test.PASS:
         res.code = Test.FLAKYPASS
     return res

llvm/utils/lit/tests/Inputs/allow-retries/does-not-succeed-within-limit.py

+# ALLOW_RETRIES: 3
+
+# RUN: false

llvm/utils/lit/tests/Inputs/allow-retries/lit.cfg

+import lit.formats
+config.name = 'allow-retries'
+config.suffixes = ['.py']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+
+config.substitutions.append(('%python', lit_config.params.get('python', '')))
+config.substitutions.append(('%counter', lit_config.params.get('counter', '')))

llvm/utils/lit/tests/Inputs/allow-retries/more-than-one-allow-retries-lines.py

+# ALLOW_RETRIES: 3
+# ALLOW_RETRIES: 5
+
+# RUN: true

llvm/utils/lit/tests/Inputs/allow-retries/not-a-valid-integer.py

+# ALLOW_RETRIES: not-an-integer
+
+# RUN: true

llvm/utils/lit/tests/Inputs/allow-retries/succeeds-within-limit.py

+# ALLOW_RETRIES: 5
+
+# RUN: "%python" "%s" "%counter"
+
+import sys
+import os
+
+counter_file = sys.argv[1]
+
+# The first time the test is run, initialize the counter to 1.
+if not os.path.exists(counter_file):
+    with open(counter_file, 'w') as counter:
+        counter.write("1")
+
+# Succeed if this is the fourth time we're being run.
+with open(counter_file, 'r') as counter:
+    num = int(counter.read())
+    if num == 4:
+        sys.exit(0)
+
+# Otherwise, increment the counter and fail
+with open(counter_file, 'w') as counter:
+    counter.write(str(num + 1))
+    sys.exit(1)

llvm/utils/lit/tests/Inputs/test_retry_attempts/lit.cfg

+import lit.formats
+config.name = 'test_retry_attempts'
+config.suffixes = ['.py']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+
+config.test_retry_attempts = 5
+config.substitutions.append(('%python', lit_config.params.get('python', '')))
+config.substitutions.append(('%counter', lit_config.params.get('counter', '')))

llvm/utils/lit/tests/Inputs/test_retry_attempts/test.py

+# RUN: "%python" "%s" "%counter"
+
+import sys
+import os
+
+counter_file = sys.argv[1]
+
+# The first time the test is run, initialize the counter to 1.
+if not os.path.exists(counter_file):
+    with open(counter_file, 'w') as counter:
+        counter.write("1")
+
+# Succeed if this is the fourth time we're being run.
+with open(counter_file, 'r') as counter:
+    num = int(counter.read())
+    if num == 4:
+        sys.exit(0)
+
+# Otherwise, increment the counter and fail
+with open(counter_file, 'w') as counter:
+    counter.write(str(num + 1))
+    sys.exit(1)

llvm/utils/lit/tests/Inputs/testrunner-custom-parsers/test.txt

 
 // MY_TAG.
 // foo bar baz
 // MY_RUN: baz
 // MY_LIST: one, two
 // MY_LIST: three, four
 // MY_RUN: foo \
 // MY_RUN: bar
 //
 // MY_CUSTOM: a b c
 //
 // MY_BOOL: a && (\
 // MY_BOOL: b)
 // MY_BOOL: d
 //
+// MY_INT: 4
+// MY_INT: 6
+//
 // MY_BOOL_UNTERMINATED: a \
 //
 // END.
 // MY_LIST: five

llvm/utils/lit/tests/allow-retries.py

+# Check the behavior of the ALLOW_RETRIES keyword.
+
+# This test uses a file that's stable across retries of the test to fail and
+# only succeed the fourth time it is retried.
+#
+# RUN: rm -f %t.counter
+# RUN: %{lit} -j 1 %{inputs}/allow-retries/succeeds-within-limit.py -Dcounter=%t.counter -Dpython=%{python} | FileCheck --check-prefix=CHECK-TEST1 %s
+# CHECK-TEST1: Passes With Retry  : 1
+
+# Test that a per-file ALLOW_RETRIES overwrites the config-wide test_retry_attempts property, if any.
+#
+# RUN: rm -f %t.counter
+# RUN: %{lit} -j 1 %{inputs}/allow-retries/succeeds-within-limit.py -Dtest_retry_attempts=2 -Dcounter=%t.counter -Dpython=%{python} | FileCheck --check-prefix=CHECK-TEST2 %s
+# CHECK-TEST2: Passes With Retry  : 1
+
+# This test does not succeed within the allowed retry limit
+#
+# RUN: not %{lit} -j 1 %{inputs}/allow-retries/does-not-succeed-within-limit.py | FileCheck --check-prefix=CHECK-TEST3 %s
+# CHECK-TEST3: Failing Tests (1):
+# CHECK-TEST3: allow-retries :: does-not-succeed-within-limit.py
+
+# This test should be UNRESOLVED since it has more than one ALLOW_RETRIES
+# lines, and that is not allowed.
+#
+# RUN: not %{lit} -j 1 %{inputs}/allow-retries/more-than-one-allow-retries-lines.py | FileCheck --check-prefix=CHECK-TEST4 %s
+# CHECK-TEST4: Unresolved Tests (1):
+# CHECK-TEST4: allow-retries :: more-than-one-allow-retries-lines.py
+
+# This test does not provide a valid integer to the ALLOW_RETRIES keyword.
+# It should be unresolved.
+#
+# RUN: not %{lit} -j 1 %{inputs}/allow-retries/not-a-valid-integer.py | FileCheck --check-prefix=CHECK-TEST5 %s
+# CHECK-TEST5: Unresolved Tests (1):
+# CHECK-TEST5: allow-retries :: not-a-valid-integer.py
+
+# This test checks that the config-wide test_retry_attempts property is used
+# when no ALLOW_RETRIES keyword is present.
+#
+# RUN: rm -f %t.counter
+# RUN: %{lit} -j 1 %{inputs}/test_retry_attempts/test.py -Dcounter=%t.counter -Dpython=%{python} | FileCheck --check-prefix=CHECK-TEST6 %s
+# CHECK-TEST6: Passes With Retry  : 1

llvm/utils/lit/tests/unit/TestRunner.py

def make_parsers():
             output += [part for part in line.split(' ') if part.strip()]
             return output
 
         return [
             IntegratedTestKeywordParser("MY_TAG.", ParserKind.TAG),
             IntegratedTestKeywordParser("MY_DNE_TAG.", ParserKind.TAG),
             IntegratedTestKeywordParser("MY_LIST:", ParserKind.LIST),
             IntegratedTestKeywordParser("MY_BOOL:", ParserKind.BOOLEAN_EXPR),
+            IntegratedTestKeywordParser("MY_INT:", ParserKind.INTEGER),
             IntegratedTestKeywordParser("MY_RUN:", ParserKind.COMMAND),
             IntegratedTestKeywordParser("MY_CUSTOM:", ParserKind.CUSTOM,
                                         custom_parse),
 
         ]
 
     @staticmethod
     def get_parser(parser_list, keyword):
def test_boolean(self):
         parsers = self.make_parsers()
         self.parse_test(parsers)
         bool_parser = self.get_parser(parsers, 'MY_BOOL:')
         value = bool_parser.getValue()
         self.assertEqual(len(value), 2)  # there are only two run lines
         self.assertEqual(value[0].strip(), "a && (b)")
         self.assertEqual(value[1].strip(), "d")
 
+    def test_integer(self):
+        parsers = self.make_parsers()
+        self.parse_test(parsers)
+        int_parser = self.get_parser(parsers, 'MY_INT:')
+        value = int_parser.getValue()
+        self.assertEqual(len(value), 2)  # there are only two MY_INT: lines
+        self.assertEqual(type(value[0]), int)
+        self.assertEqual(value[0], 4)
+        self.assertEqual(type(value[1]), int)
+        self.assertEqual(value[1], 6)
+
     def test_boolean_unterminated(self):
         parsers = self.make_parsers() + \
             [IntegratedTestKeywordParser("MY_BOOL_UNTERMINATED:", ParserKind.BOOLEAN_EXPR)]
         try:
             self.parse_test(parsers)
             self.fail('expected exception')
         except ValueError as e:
             self.assertIn("Test has unterminated MY_BOOL_UNTERMINATED: lines", str(e))

llvm/utils/vim/syntax/llvm.vim

 syn match   llvmConstant /\<DW_LANG_[a-zA-Z0-9_]\+\>/
 syn match   llvmConstant /\<DW_VIRTUALITY_[a-z_]\+\>/
 syn match   llvmConstant /\<DIFlag[A-Za-z]\+\>/
 
 " Syntax-highlight lit test commands and bug numbers.
 syn match  llvmSpecialComment /;\s*PR\d*\s*$/
 syn match  llvmSpecialComment /;\s*REQUIRES:.*$/
 syn match  llvmSpecialComment /;\s*RUN:.*$/
+syn match  llvmSpecialComment /;\s*ALLOW_RETRIES:.*$/
 syn match  llvmSpecialComment /;\s*CHECK:.*$/
 syn match  llvmSpecialComment "\v;\s*CHECK-(NEXT|NOT|DAG|SAME|LABEL):.*$"
 syn match  llvmSpecialComment /;\s*XFAIL:.*$/
 
 if version >= 508 || !exists("did_c_syn_inits")
   if version < 508
     let did_c_syn_inits = 1
     command -nargs=+ HiLink hi link <args>

llvm/utils/vscode/llvm/syntaxes/ll.tmLanguage.yaml

- match: "\\bDW_TAG_[a-z_]+\\b|\
             \\bDIFlag[A-Za-z]+\\b"
     name: constant.other
   - match: ";\\s*PR\\d*\\s*$"
     name: string.regexp
   - match: ";\\s*REQUIRES:.*$"
     name: string.regexp
   - match: ";\\s*RUN:.*$"
     name: string.regexp
+  - match: ";\\s*ALLOW_RETRIES:.*$"
+    name: string.regexp
   - match: ";\\s*CHECK:.*$"
     name: string.regexp
   - match: ";\\s*CHECK-(NEXT|NOT|DAG|SAME|LABEL):.*$"
     name: string.regexp
   - match: ";\\s*XFAIL:.*$"
     name: string.regexp
   - match: ;.*$
     name: comment.line.llvm