EXPECT_THAT with Throws invokes callable twice when exception is not thrown

[dragon-dreamer]

Describe the bug

EXPECT_THAT macro with the Throws expectation invokes the callable twice when it does not throw an exception. This does not happen when the exception is thrown.

This causes failures for stateful lambdas or lambdas with side effects.

Steps to reproduce the bug

Repro code:

#include <exception>
#include <iostream>
#include <gmock/gmock.h>

int main()
{
    int state = 1;
    EXPECT_THAT(([&] { std::cout << "I am invoked " << state++ << '\n'; }),
        testing::Throws<std::exception>());
}

Outputs:

I am invoked 1
I am invoked 2
...

Also see https://godbolt.org/z/WE44hn5Kc.

Does the bug persist in the most recent commit?

Yes.

What operating system and version are you using?

Does not matter. The issue happens on both Windows and Linux.

What compiler and version are you using?

MSVC 17.4.1, clang trunk.

[kohyida1997]

Just wanted to chip in after digging through the source code and inspecting the compiled assembly (I'm not a maintainer of this code):

It appears this is expected behavior based on the source code in https://github.com/google/googletest/blob/main/googlemock/include/gmock/gmock-matchers.h.

The EXPECT_THAT expands into a call to PredicateFormatterFromMatcher::operator() with the lambda passed in as a template type. See:
Lines: 5450 - 5452

#define EXPECT_THAT(value, matcher) \
  EXPECT_PRED_FORMAT1(              \
      ::testing::internal::MakePredicateFormatterFromMatcher(matcher), value)

Inside the PredicateFormatterFromMatcher::operator() function:

Lines: 1600 - 1607

    // Rerun the matcher to "PrintAndExplain" the failure.
    StringMatchResultListener listener;
    if (MatchPrintAndExplain(x, matcher, &listener)) { // this runs if matcher fails
      ss << "\n  The matcher failed on the initial attempt; but passed when "
            "rerun to generate the explanation.";
    }
    ss << "\n  Actual: " << listener.str();
    return AssertionFailure() << ss.str();

Only when the assertion fails, then is the Matcher is re-run to generate the error message. This explains why the lambda does not execute twice when it successfully throws an exception. See:

Lines 1591-1593

    if (matcher.Matches(x)) { // Early exit if matcher passes
      return AssertionSuccess();
    }

Hope this helps somebody!

[higher-performance]

The cause is clear enough, but it's not clear to me how feasible it is to avoid it unfortunately. I tried what looked like an obvious fix, but it seems trickier than I thought in the general case. I'll keep the issue open for now in case others have thoughts, but I'm not sure when I might get to take another look. If you find a fix you're also welcome to open a pull request.

[kohyida1997]

The cause is clear enough, but it's not clear to me how feasible it is to avoid it unfortunately. I tried what looked like an obvious fix, but it seems trickier than I thought in the general case. I'll keep the issue open for now in case others have thoughts, but I'm not sure when I might get to take another look. If you find a fix you're also welcome to open a pull request.

Out of curiosity - what was the "obvious fix"?

[higher-performance]

Removing the if (matcher.Matches(x)) block and deferring stringization like ss << "Value of: " so that they are avoided when the match is successful. That worked for many cases I saw, but it ended up breaking some things.

[higher-performance]

By the way, have you tried using EXPECT_THROW? On my tests, it seems to only invoke the callable once:

EXPECT_THROW((std::cout << "I am invoked " << state++ << '\n'), std::exception)

Is this adequate for what you're trying to do?

Sorry for the lack of a better fix—in the meantime, though I would suggest using that as a workaround. If you run into this issue somewhere where EXPECT_THROW isn't a practical workaround, please let us know. Thanks for reporting!

[dragon-dreamer]

Thank you for providing some context.

Yes, I tried EXPECT_THROW, but unfortunately, it is too limited for my use case: I need to inspect the exception message and some fields of the exception object, which I do with a custom matcher. I believe, EXPECT_THROW does not allow to apply any matchers to the exception object, but only allows to check the exception class.

I have other workarounds (like re-creating the full state inside the lambda body), which should help in most of my cases, but this is obviously not ideal.

[higher-performance]

Ah I see. I'll keep this issue open in case someone can find a solution at some point, but unfortunately you'll have to work around it for now. Thanks!

[Hsilgos]

The same issue occurs when exception doesn't match condition (for example text is different).
My naive attempt to add bool flag to call lambda once didn't help because in this case despite lambda is called once and throws exception gmock prints that no exception was thrown.
What else can be done? Store current exception and rethrow on second call?

UPD:
Here is my workaround (seems good enough for me):

template<class Function>
std::function<void()> single_call(Function function)
{
    auto shared_exception_ptr = std::make_shared<std::exception_ptr>();
    auto was_called = std::make_shared<bool>(false);
    return [shared_exception_ptr, was_called, function]() {
        if (*shared_exception_ptr) {
            std::rethrow_exception(*shared_exception_ptr);
        }
        if (*was_called) {
            return;
        }
        *was_called = true;
        try {
            std::invoke(function);
        } catch (...) {
            *shared_exception_ptr = std::current_exception();
            std::rethrow_exception(*shared_exception_ptr);
        }
    };
}

Usage:

EXPECT_THAT(single_call([&] {  throw std::logic_error("Blah") }),
                Throws<std::logic_error>(Property(&std::logic_error::what, Eq("Blah"))));

[rturrado]

Only when the assertion fails, then is the Matcher is re-run to generate the error message.
Hope this helps somebody!

It has just helped me indeed, thanks! I had changed my error messages, then a test was failing, and it was driving me crazy why the callable was being invoked twice.

[Hsilgos]

My workaround:

template <class Function>
std::function<void()>
single_call(Function function)
{
    auto shared_exception_ptr = std::make_shared<std::exception_ptr>();
    auto was_called = std::make_shared<bool>(false);
    return [shared_exception_ptr, was_called, function]() {
        if (*shared_exception_ptr)
        {
            std::rethrow_exception(*shared_exception_ptr);
        }
        if (*was_called)
        {
            return;
        }
        *was_called = true;
        try
        {
            std::invoke(function);
        }
        catch (...)
        {
            *shared_exception_ptr = std::current_exception();
            std::rethrow_exception(*shared_exception_ptr);
        }
    };
}

Usage:

EXPECT_THAT(single_call([] { throw std::out_of_range("Blah"); }),
                             Throws<std::logic_error>());

[huang325]

Removing the if (matcher.Matches(x)) block and deferring stringization like ss << "Value of: " so that they are avoided when the match is successful. That worked for many cases I saw, but it ended up breaking some things.

Looking into this bug, and just querious about this test case. Seems it is designed to have a second chance here. Is this truly a bug or a design choice? I am just wondering what's the original reason to have this second chance here. Is that plan to work for any special case?

[sburton84]

I've been running into this issue also. In my case, my lambda function uses std::move to take ownership of a particular unique_ptr, which means by the time the function runs a second time this pointer is now null and causes my tests to crash when it tries to dereference it. Obviously this seems like quite unexpected behaviour; it would really be better if the callable was only called once.

[XAMeLeOH]

Is this truly a bug or a design choice?

@huang325, I believe it's the latter. The documentation says:

WARNING: gMock does not guarantee when or how many times a matcher will be invoked. Therefore, all matchers must be purely functional: they cannot have any side effects, and the match result must not depend on anything other than the matcher’s parameters and the value being matched.

Also here.