[Bug]: --gtest_break_on_failure conflicts with EXPECT_NONFATAL_FAILURE

[bklarson]

Describe the issue

I have a test which uses EXPECT_NONFATAL_FAILURE.

I've recently started using --gtest_repeat (which is fantastic) and want to use --gtest_break_on_failure for my debugger to track down a rare timing issue.

It seems that break_on_failure is not checking that we are inside an EXPECT_NONFATAL_FAILURE clause and breaking execution.

Steps to reproduce the problem

See example at https://godbolt.org/z/rETraz1Pv

Remove the --gtest_break_on_failure command-line argument and the test will now pass

What version of GoogleTest are you using?

1.10.0 (Sorry, this is all that is built in to godbolt, I don't have a fast way to test with a newer version)

What operating system and version are you using?

godbolt (linux)

What compiler and version are you using?

GCC 10.4 x86

What build system are you using?

godbolt

Additional context

No response

[thestarivore]

Hi, I'd like to work on this issue.

[avrdan]

@bklarson @derekmauro Break on failure seems to currently break on both regular failure and non fatal failure. If this is was not the intended design, all you have to do is to add the missing check in gtest.cc:5354:

  if (result_type != TestPartResult::kSuccess &&
      result_type != TestPartResult::kSkip &&
      result_type != TestPartResult::kNonFatalFailure) {

It now doesn't break (SIGTRAP) for non-fatal failure.

The question is then if this is the correct behavior for throw_on_failure, to also ignore non fatal failures. Using --gtest_throw_on_failure not throw with the above condition. Otherwise we need to move the condition to the line:
if (GTEST_FLAG_GET(break_on_failure) && result_type != TestPartResult::kNonFatalFailure) {

[higher-performance]

Does this proposed solution pass all the tests locally (such as this one)?

[avrdan]

@higher-performance the googletest-throw-on-failure-test_.cc and googletest-break-on-failure-unittest_.cc are excluded from the regular test build process in the build script. All the non-excluded tests pass.

The break-on-failure test fails (expected) and produces all output (doesn't seem to do much on non-Windows machines), while the throw-on-failure test just says "Failure" and then output is interrupted due to the throw (expected). I don't see "Unhandled C++ exception terminating the program.", but probably because stderr is not printed by default.

[higher-performance]

Thanks for the reply. It's been a while since I looked at this issue, but I'm skeptical the solution is this simple... but perhaps I'm missing something? If this is the solution, then existing tests should all pass. (Unless they're buggy, in which case those bugs should be fixed.) I'm not entirely sure what you mean by failures being "expected" here, but it sounds to me like this would make some current tests start failing, which sounds to me like this breaks something, which in any case we don't want. So if you do see failures, they'd need to be addressed before we can consider this solution?

If I misunderstood something let me know - I've lost some context since looking at this earlier.

P.S. How does the gtest_throw_on_failure_ex_test do with your changes?

[avrdan]

You misunderstood. Some tests are intended to fail or have non-standard behavior, that is probably why they are in the exclusion list in the bazel build. Only the break one fails, the throw tests succeeds, despite the failure. So far as I can tell, the behavior I have seen for the previously mentioned two tests is ok.

The solution is indeed simple. There is a separate state for non-fatal failures. The same happens for gtest_throw_on_failure_ex_test.cc. Here is the output:

googletest/test/gtest_throw_on_failure_ex_test.cc:64: Failure
Expected equality of these values:
  2
  3
Expected failure

This is expected since:
if (strstr(e.what(), "Expected failure") != nullptr) { return; }
Also, I have added a print before the return and it does print it, so for this test, it just throws a runtime_error that is caught.

And here is the output for googletest-throw-on-failure-test_:

googletest/test/googletest-throw-on-failure-test_.cc:66: Failure
Expected equality of these values:
  2
  3

For reference, here is the exclusion list:

        "gtest-unittest-api_test.cc",
        "googletest/src/gtest-all.cc",
        "gtest_all_test.cc",
        "gtest-death-test_ex_test.cc",
        "gtest-listener_test.cc",
        "gtest-unittest-api_test.cc",
        "googletest-param-test-test.cc",
        "googletest-param-test2-test.cc",
        "googletest-catch-exceptions-test_.cc",
        "googletest-color-test_.cc",
        "googletest-env-var-test_.cc",
        "googletest-failfast-unittest_.cc",
        "googletest-filter-unittest_.cc",
        "googletest-global-environment-unittest_.cc",
        "googletest-break-on-failure-unittest_.cc",
        "googletest-listener-test.cc",
        "googletest-message-test.cc",
        "googletest-output-test_.cc",
        "googletest-list-tests-unittest_.cc",
        "googletest-shuffle-test_.cc",
        "googletest-setuptestsuite-test_.cc",
        "googletest-uninitialized-test_.cc",
        "googletest-death-test_ex_test.cc",
        "googletest-param-test-test",
        "googletest-throw-on-failure-test_.cc",
        "googletest-param-test-invalid-name1-test_.cc",
        "googletest-param-test-invalid-name2-test_.cc",

[avrdan]

Actually I think the TerminateHandler is not called for googletest-throw-on-failure-test_, which is a problem. I get the same behavior, regardless of whether GTEST_HAS_EXCEPTIONS is used.

I think the flag is missing: GTEST_FLAG_SET(throw_on_failure, true);

Again, all the tests on the non-exclusion list pass - 408 tests run, 407 tests passed 1 skipped

[avrdan]

Actually there is indeed a problem, with my code, the gtest_throw_on_failure_ex_test catches it. The other one is missing the flag. We need to add GTEST_FLAG_SET(throw_on_failure, true); to fix the test. Let me know if you agree.

I changed the code so that it only impacts the break_on_failure condition and now both tests behave correctly (after amending the test):

if (GTEST_FLAG_GET(break_on_failure) &&
      result_type != TestPartResult::kNonFatalFailure) {

Shouldn't we need a new test for this? The googletest-break-on-failure-unittest_ doesn't seem to be doing what is required for this bug (I tested manually using a new test according to OP's godbolt).

[higher-performance]

Actually there is indeed a problem, with my code, the gtest_throw_on_failure_ex_test catches it. The other one is missing the flag. We need to add GTEST_FLAG_SET(throw_on_failure, true); to fix the test. Let me know if you agree.

At the moment I'm not sure; unfortunately we haven't had the time to look at this issue carefully (hence the lack of progress here). For what it's worth, I did take a stab at a solution for this issue early on, and it required a fair bit of time investment for me to feel confident it's correct, but the solution I drafted was much more complicated than this. It's possible I've missed something, but at the moment I don't feel the solution is likely to be this simple.

Shouldn't we need a new test for this?

New tests would need to be part of any solution, yes. I don't think we test all the edge cases currently. You'll want to do that exhaustively to convince yourself (and us/others) that the patch is correct.