[Bug]: documented undefined behavior seems to make common use of fixtures illegal

[Griffon26]

Describe the issue

I first submitted this issue under discussions, but I think it is actually a bug. Unless I misunderstand the documentation, the defined behavior of EXPECT_CALL limits the usability of fixtures to the point of uselessness.

As the cookbook mentions it is good practice to verify one thing in one test. In order to do this, there is often some setup required before a test can perform the calls it is interested in.

The problem comes when that setup leads to calls on a mock that the test later wants to set EXPECTations on.
As the cookbook says: "gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined."
Is there a way to avoid undefined behavior and still have useful fixtures?

Things I have considered:

• putting expectations before the setup is undesirable because it not only makes it impossible to put the setup in the fixture, but it also requires explicitly writing expectations for stuff that happens in the setup, which should not be the concern of the test case
• as is mentioned in Is interleaving EXPECT_CALL()s and calls to the mock function *really* undefined behavior? #2828 (comment) we could recreate the mocks in place to get around UB. This doesn't sound like the easiest possible solution.
• add functions to product code that allow test code to directly set the desired state. This is undesirable because you don't want product code that only exists for the purpose of testing.

Steps to reproduce the problem

Given a class S that calls method f of class D

• create a test with a fixture for class S with a mock for class D
• in the setup of the fixture call some methods on S that will lead to calls of D::f
• in the test case place an expectation on a call to D::f -> undefined behavior according to https://google.github.io/googletest/gmock_for_dummies.html#using-mocks-in-tests

I could give code, but code can't be used to show that behavior is undefined.

What version of GoogleTest are you using?

I'm basing this on the latest documentation at https://google.github.io/googletest/gmock_for_dummies.html#using-mocks-in-tests

What operating system and version are you using?

n.a.

What compiler and version are you using?

n.a.

What build system are you using?

n.a.

Additional context

n.a.

[Griffon26]

@dinord since you self-assigned this, any thoughts on this?

[dinord]

We'll discuss this internally next week.

[dinord]

We discovered that even some of GoogleTest's own unit tests depend on this kind of undefined behavior. Still, there's some debate as to whether defining this behavior would encourage undesired patterns and how to make this thread-safe.

I'll get back to this issue once I have more internal feedback about those.

[dinord]

We need to do some work internally before moving forward with this issue, and we haven't had the chance to prioritize it yet.

[bloerwald]

We need to do some work internally before moving forward with this issue, and we haven't had the chance to prioritize it yet.

Are you able to share any progress on this?

[mliszcz]

I found this while trying to understand the reason behind the UB:

googletest/googlemock/include/gmock/gmock-spec-builders.h

Lines 226 to 236 in eff443c

	// All expectations for this function mocker.
	//
	// It's undefined behavior to interleave expectations (EXPECT_CALLs
	// or ON_CALLs) and mock function calls. Also, the order of
	// expectations is important. Therefore it's a logic race condition
	// to read/write untyped_expectations_ concurrently. In order for
	// tools like tsan to catch concurrent read/write accesses to
	// untyped_expectations, we deliberately leave accesses to it
	// unprotected.
	UntypedExpectations untyped_expectations_;
	}; // class UntypedFunctionMockerBase

Indeed during the EXPEC_CALL calls and the calls to the mocked method the lock is not acquired:

googletest/googlemock/include/gmock/gmock-spec-builders.h

Lines 1578 to 1580 in eff443c

	// See the definition of untyped_expectations_ for why access to
	// it is unprotected here.
	untyped_expectations_.push_back(untyped_expectation);

googletest/googlemock/include/gmock/gmock-spec-builders.h

Lines 1789 to 1793 in eff443c

	R FunctionMocker<R(Args...)>::InvokeWith(ArgumentTuple&& args)
	GTEST_LOCK_EXCLUDED_(g_gmock_mutex) {
	// See the definition of untyped_expectations_ for why access to it
	// is unprotected here.
	if (untyped_expectations_.size() == 0) {

@dinord can you at least confirm that EXPECT_CALL order restriction is to prevent UB due to data races? Many projects do not have multithreaded UTs and do not care at all about internal locks in gmock. If this is the case, then why don't simply state something like: "the Mock classes are not threadsafe, please handle locking on your own".