Page MenuHomePhabricator

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

Authored by stephenneuendorffer on Sun, May 17, 5:45 PM.

Details

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSun, May 17, 5:45 PM

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 added a comment.Mon, May 18, 10:49 AM

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

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 accepted this revision.Tue, May 19, 11:41 PM

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

This revision is now accepted and ready to land.Tue, May 19, 11:41 PM
stephenneuendorffer marked 4 inline comments as done.Wed, May 20, 1:35 PM
This revision was automatically updated to reflect the committed changes.

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