[lit] Fix handling of various keyword parse errors

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.

Reviewed By: ldionne

Differential Revision: https://reviews.llvm.org/D81665

llvm/utils/lit/lit/TestRunner.py

@@ -1401,7 +1401,7 @@ def _parseKeywords(sourcepath, additional_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"
@@ -1419,12 +1419,11 @@ def _parseKeywords(sourcepath, additional_parsers=[],
 
     # 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:
@@ -1433,13 +1432,13 @@ def _parseKeywords(sourcepath, additional_parsers=[],
             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()}
 
@@ -1458,7 +1457,11 @@ def parseIntegratedTestScript(test, additional_parsers=[],
     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 []

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

@@ -0,0 +1,4 @@
+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

@@ -0,0 +1,3 @@
+ALLOW_RETRIES: 1
+ALLOW_RETRIES: 1
+RUN: echo

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

@@ -0,0 +1,3 @@
+# RUN: echo \
+
+Program code.

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

@@ -0,0 +1,15 @@
+# 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

@@ -72,13 +72,16 @@ class TestIntegratedTestKeywordParser(unittest.TestCase):
         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)
-        assert isinstance(script, list)
-        assert len(script) == 0
+        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()
@@ -124,15 +127,34 @@ class TestIntegratedTestKeywordParser(unittest.TestCase):
         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()