[lit] Fix handling of various keyword parse errors

[jdenny]

Summary

In TestRunner.py, D78589 extracts a _parseKeywords function from
parseIntegratedTestScript, which then expects _parseKeywords to
always return a list of keyword/value pairs. However, the extracted
code sometimes returns an unresolved lit.Test.Result on a keyword
parsing error, which then produces a stack dump instead of the
expected diagnostic.

This patch fixes that, makes the style of those diagnostics more
consistent, and extends the lit test suite to cover them.

[ldionne]

Ah, nice catch. Thanks for fixing this.

[jdenny]

Thanks for the quick review.

llvm/utils/lit/lit/TestRunner.py

builtin_parsers = [
         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 '
+            raise ValueError('Additional parser must be an instance of '
                              'IntegratedTestKeywordParser')
         if parser.keyword in keyword_parsers:
             raise ValueError("Parser for keyword '%s' already exists"
                              % parser.keyword)
         keyword_parsers[parser.keyword] = parser
 
     # Collect the test lines from the script.
     for line_number, command_type, ln in \
             parseIntegratedTestScriptCommands(sourcepath,
                                               keyword_parsers.keys()):
         parser = keyword_parsers[command_type]
         parser.parseLine(line_number, ln)
         if command_type == 'END.' and parser.getValue() is True:
             break
 
     # Verify the script contains a run line.
     if require_script and not script:
-        return lit.Test.Result(Test.UNRESOLVED, "Test has no run line!")
+        raise ValueError("Test has no 'RUN:' line")
 
     # Check for unterminated run lines.
     if script and script[-1][-1] == '\\':
-        return lit.Test.Result(Test.UNRESOLVED,
-                               "Test has unterminated run lines (with '\\')")
+        raise ValueError("Test has unterminated 'RUN:' lines (with '\\')")
 
     # Check boolean expressions for unterminated lines.
     for key in keyword_parsers:
         kp = keyword_parsers[key]
         if kp.kind != ParserKind.BOOLEAN_EXPR:
             continue
         value = kp.getValue()
         if value and value[-1][-1] == '\\':
-            raise ValueError("Test has unterminated %s lines (with '\\')" % key)
+            raise ValueError("Test has unterminated '{key}' lines (with '\\')"
+                             .format(key=key))
 
     # Make sure there's at most one ALLOW_RETRIES: line
     allowed_retries = keyword_parsers['ALLOW_RETRIES:'].getValue()
     if allowed_retries and len(allowed_retries) > 1:
-        return lit.Test.Result(Test.UNRESOLVED,
-                               "Test has more than one ALLOW_RETRIES lines")
+        raise ValueError("Test has more than one ALLOW_RETRIES lines")
 
     return {p.keyword: p.getValue() for p in keyword_parsers.values()}
 
 
 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', 'REQUIRES',
     'UNSUPPORTED' and 'ALLOW_RETRIES' information into the given test.
 
     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.
     """
     # Parse the test sources and extract test properties
-    parsed = _parseKeywords(test.getSourcePath(), additional_parsers, require_script)
+    try:
+        parsed = _parseKeywords(test.getSourcePath(), additional_parsers,
+                                require_script)
+    except ValueError as e:
+        return lit.Test.Result(Test.UNRESOLVED, str(e))
     script = parsed['RUN:'] or []
     test.xfails = parsed['XFAIL:'] or []
     test.requires = parsed['REQUIRES:'] or []
     test.unsupported = parsed['UNSUPPORTED:'] or []
     if parsed['ALLOW_RETRIES:']:
         test.allowed_retries = parsed['ALLOW_RETRIES:'][0]
 
     # Enforce REQUIRES:

llvm/utils/lit/tests/Inputs/shtest-keyword-parse-errors/lit.cfg

+import lit.formats
+config.name = 'shtest-keyword-parse-errors'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()

llvm/utils/lit/tests/Inputs/shtest-keyword-parse-errors/multiple-allow-retries.txt

+ALLOW_RETRIES: 1
+ALLOW_RETRIES: 1
+RUN: echo

llvm/utils/lit/tests/Inputs/shtest-keyword-parse-errors/unterminated-run.txt

+# RUN: echo \
+
+Program code.

llvm/utils/lit/tests/shtest-keyword-parse-errors.py

+# RUN: not %{lit} -j 1 -vv %{inputs}/shtest-keyword-parse-errors > %t.out
+# RUN: FileCheck -input-file %t.out %s
+#
+# END.
+
+# CHECK: Testing: 3 tests
+
+# CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: empty.txt
+# CHECK:       {{^}}Test has no 'RUN:' line{{$}}
+
+# CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: multiple-allow-retries.txt
+# CHECK:       {{^}}Test has more than one ALLOW_RETRIES lines{{$}}
+
+# CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: unterminated-run.txt
+# CHECK:       {{^}}Test has unterminated 'RUN:' lines (with '\'){{$}}

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

class TestIntegratedTestKeywordParser(unittest.TestCase):
     @staticmethod
     def get_parser(parser_list, keyword):
         for p in parser_list:
             if p.keyword == keyword:
                 return p
         assert False and "parser not found"
 
     @staticmethod
-    def parse_test(parser_list):
+    def parse_test(parser_list, allow_result=False):
         script = parseIntegratedTestScript(
             TestIntegratedTestKeywordParser.inputTestCase,
             additional_parsers=parser_list, require_script=False)
-        assert not isinstance(script, lit.Test.Result)
+        if isinstance(script, lit.Test.Result):
+            assert allow_result
+        else:
             assert isinstance(script, list)
             assert len(script) == 0
+        return script
 
     def test_tags(self):
         parsers = self.make_parsers()
         self.parse_test(parsers)
         tag_parser = self.get_parser(parsers, 'MY_TAG.')
         dne_tag_parser = self.get_parser(parsers, 'MY_DNE_TAG.')
         self.assertTrue(tag_parser.getValue())
         self.assertFalse(dne_tag_parser.getValue())
def test_integer(self):
         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_bad_parser_type(self):
+        parsers = self.make_parsers() + ["BAD_PARSER_TYPE"]
+        script = self.parse_test(parsers, allow_result=True)
+        self.assertTrue(isinstance(script, lit.Test.Result))
+        self.assertEqual(script.code, lit.Test.UNRESOLVED)
+        self.assertEqual('Additional parser must be an instance of '
+                         'IntegratedTestKeywordParser',
+                         script.output)
+
+    def test_duplicate_keyword(self):
+        parsers = self.make_parsers() + \
+            [IntegratedTestKeywordParser("KEY:", ParserKind.BOOLEAN_EXPR),
+             IntegratedTestKeywordParser("KEY:", ParserKind.BOOLEAN_EXPR)]
+        script = self.parse_test(parsers, allow_result=True)
+        self.assertTrue(isinstance(script, lit.Test.Result))
+        self.assertEqual(script.code, lit.Test.UNRESOLVED)
+        self.assertEqual("Parser for keyword 'KEY:' already exists",
+                         script.output)
+
     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))
-
+        script = self.parse_test(parsers, allow_result=True)
+        self.assertTrue(isinstance(script, lit.Test.Result))
+        self.assertEqual(script.code, lit.Test.UNRESOLVED)
+        self.assertEqual("Test has unterminated 'MY_BOOL_UNTERMINATED:' lines "
+                         "(with '\\')",
+                         script.output)
 
     def test_custom(self):
         parsers = self.make_parsers()
         self.parse_test(parsers)
         custom_parser = self.get_parser(parsers, 'MY_CUSTOM:')
         value = custom_parser.getValue()
         self.assertEqual(value, ['a', 'b', 'c'])