[lit] GoogleTest framework should report failures if test binary crashes

[stephenneuendorffer]

Summary

lit runs a gtest executable multiple times. First it runs it to
discover tests, then later it runs the executable again for each test.
However, if the discovery fails (perhaps because of a broken
executable), then no tests were previously run and no failures were
reported. This patch creates a dummy test if discovery fails, which
will later fail when test are run and be reported as a failure.

[mehdi_amini]

LGTM, but I'm not sure about possible side-effects here. No one worked recently on this file, but Julian worked on lit in general recently and maybe can sanity check this as well?

[yln]

The commit message and comment sound reasonable, but I have not further insights. Is it possible to capture the improved behavior with a test?

[stephenneuendorffer]

In D80096#2042190, @yln wrote:

The commit message and comment sound reasonable, but I have not further insights. Is it possible to capture the improved behavior with a test?

good point. I added one. Thanks for the review!

[yln]

LGTM, with nits.

llvm/utils/lit/tests/Inputs/googletest-brokendiscovery/DummySubDir/OneTest.py

4 ↗

(On Diff #265063)

Unnecessary imports

llvm/utils/lit/tests/Inputs/googletest-brokendiscovery/lit.cfg

2 ↗

(On Diff #265063)

How about naming the test "googletest-error-during-discovery" or "googletest-disovery-failure" since discovery itself isn't "broken"?

llvm/utils/lit/tests/googletest-brokendiscovery.py

1 ↗

(On Diff #265063)

Copy & pasted comment

9 ↗

(On Diff #265063)

Maybe just name the dir "subdir" to avoid the need for this regex

[tstellar]

Thanks for fixing this. I have hit this issue a few times.

llvm/utils/lit/lit/formats/googletest.py

def getGTestTests(self, path, litConfig, localConfig):
 
         try:
             output = subprocess.check_output(list_test_cmd,
                                              env=localConfig.environment)
         except subprocess.CalledProcessError as exc:
             litConfig.warning(
                 "unable to discover google-tests in %r: %s. Process output: %s"
                 % (path, sys.exc_info()[1], exc.output))
+            # This doesn't look like a valid gtest file.  This can
+            # have a number of causes, none of them good.  For
+            # instance, we could have created a broken executable.
+            # Alternatively, someone has cruft in their test
+            # directory.  If we don't return a test here, then no
+            # failures will get reported, so return a dummy test name
+            # so that the failure is reported later.
+            yield 'failed_to_discover_tests_from_gtest'
             return
 
         nested_tests = []
         for ln in output.splitlines(False):  # Don't keep newlines.
             ln = lit.util.to_string(ln)
 
             if 'Running main() from gtest_main.cc' in ln:
                 # Upstream googletest prints this to stdout prior to running

llvm/utils/lit/tests/Inputs/googletest-discovery-failed/lit.cfg

+import lit.formats
+config.name = 'googletest-discovery-failed'
+config.test_format = lit.formats.GoogleTest('subdir', 'Test')

llvm/utils/lit/tests/Inputs/googletest-discovery-failed/subdir/OneTest.py

+#!/usr/bin/env python
+
+raise SystemExit("We are not a valid gtest executable!")

llvm/utils/lit/tests/googletest-discovery-failed.py

+# Check for correct error message when discovery of tests fails.
+#
+# RUN: not %{lit} -j 1 -v %{inputs}/googletest-discovery-failed > %t.cmd.out
+# RUN: FileCheck < %t.cmd.out %s
+
+
+# CHECK: -- Testing:
+# CHECK: Failing Tests (1):
+# CHECK:   googletest-discovery-failed :: subdir/OneTest.py/failed_to_discover_tests_from_gtest
+# CHECK: Unexpected Failures: 1