GH-47659: [C++] Fix Arrow Flight Testing's unresolved external symbol error

[alinaliBQ]

Rationale for this change

Fixes issue at #47659

What changes are included in this PR?

Include add gmock as a shared private link library to arrow_flight_testing

Are these changes tested?

Build for arrow_flight_testing succeeds on my Windows environment

Are there any user-facing changes?

No

• GitHub Issue: [C++] unresolved external symbol from missing gtest mock link on Windows #47659

[github-actions]

⚠️ GitHub issue #47659 has been automatically assigned in GitHub to PR creator.

[alinaliBQ]

Hi @kou and @lidavidm, this PR is ready for review, it is a small change extracted from #46099. Our team tripped on this when developing #46099 and found a fix for this.

[kou]

Can we solve this by the following?

diff --git a/cpp/src/arrow/flight/test_definitions.cc b/cpp/src/arrow/flight/test_definitions.cc
index c6b8e2b422..5979dd76eb 100644
--- a/cpp/src/arrow/flight/test_definitions.cc
+++ b/cpp/src/arrow/flight/test_definitions.cc
@@ -39,7 +39,6 @@
 #include "arrow/util/checked_cast.h"
 #include "arrow/util/config.h"
 #include "arrow/util/logging.h"
-#include "gmock/gmock.h"
 
 #if defined(ARROW_CUDA)
 #  include "arrow/gpu/cuda_api.h"

[alinaliBQ]

Hi @kou, I have tried out the solution and the errors came back after undoing the changes at cpp\src\arrow\flight\CMakeLists.txt. And it seems the include of gmock at test_definitions.cc is not necessary and can be removed, so I removed it.

I then found there is also #include <gmock/gmock.h> at
cpp\src\arrow\flight\test_util.h. And after trying to remove the gmock.h reference at test_util.h, I found that the mock include is required for the file test_util.h.

[kou]

Ah, OK.

arrow/cpp/src/arrow/flight/test_util.h

Line 57 in 19e3f90

ASSERT_THAT(actual.endpoints(), ::testing::ContainerEq(expected.endpoints()));

uses GMock.

Could you use SHARED_LINK_LIBS not SHARED_PRIVATE_LINK_LIBS? GMock must be a public dependency because it's used in .h not .cc.

[WillAyd]

Isn't the gmock dependency already satisfied through the use of ARROW_FLIGHT_TEST_LINK_LIBS? One thing that's a little risky in providing GMOCK as a public dependency is that when it is combined with GTEST_MAIN on Windows it has the very surprising behavior of causing tests to not run at all (see #47434)

[kou]

Ah, should we move

arrow/cpp/src/arrow/flight/test_util.h

Lines 46 to 58 in 19e3f90

	inline void AssertEqual(const FlightInfo& expected, const FlightInfo& actual) {
	ipc::DictionaryMemo expected_memo;
	ipc::DictionaryMemo actual_memo;
	ASSERT_OK_AND_ASSIGN(auto ex_schema, expected.GetSchema(&expected_memo));
	ASSERT_OK_AND_ASSIGN(auto actual_schema, actual.GetSchema(&actual_memo));
	AssertSchemaEqual(*ex_schema, *actual_schema);
	ASSERT_EQ(expected.total_records(), actual.total_records());
	ASSERT_EQ(expected.total_bytes(), actual.total_bytes());
	ASSERT_EQ(expected.descriptor(), actual.descriptor());
	ASSERT_THAT(actual.endpoints(), ::testing::ContainerEq(expected.endpoints()));
	}

to test_util.cc and don't include gmock/gmock.h in our *.h?

[WillAyd]

Worth a shot!

[alinaliBQ]

@kou I have moved AssertEqual definition to test_util.cc and checked that gmock/gmock.h is not included in any *.h test files. But the 12 errors still occur without gmock dependency being added. I found that arrow_testing lib uses gmock links directly (here) so I went with this approach and used SHARED_LINK_LIBS and removed the SHARED_PRIVATE_LINK_LIBS.

arrow/cpp/src/arrow/CMakeLists.txt

Lines 952 to 953 in 13c2615

	list(APPEND ARROW_TESTING_SHARED_LINK_LIBS ${ARROW_GTEST_GMOCK})
	list(APPEND ARROW_TESTING_STATIC_LINK_LIBS ${ARROW_GTEST_GMOCK})

@WillAyd I found ARROW_FLIGHT_TESTING_<STATIC | SHARED>_LINK_LIBS are not linked to ARROW_TEST_<STATIC | SHARED>_LINK_LIBS. The arrow library arrow_flight_testing is linked with ARROW_FLIGHT_TESTING_<STATIC | SHARED>_LINK_LIBS, not ARROW_FLIGHT_TEST_LINK_LIBS, so we had the gmock dependency issue.
If my understanding is correct, #47434 impacts tests created by add_arrow_test more and doesn't impact libraries created by add_arrow_lib. Tests have a .exe test executable (where tests were not run) and arrow_flight_testing library gives a dll file on Windows.

Thanks both for providing suggestions and info

[WillAyd]

@WillAyd I found ARROW_FLIGHT_TESTING_<STATIC | SHARED>_LINK_LIBS are not linked to ARROW_TEST_<STATIC | SHARED>_LINK_LIBS. The arrow library arrow_flight_testing is linked with ARROW_FLIGHT_TESTING_<STATIC | SHARED>_LINK_LIBS, not ARROW_FLIGHT_TEST_LINK_LIBS, so we had the gmock dependency issue.
If my understanding is correct, #47434 impacts tests created by add_arrow_test more and doesn't impact libraries created by add_arrow_lib. Tests have a .exe test executable (where tests were not run) and arrow_flight_testing library gives a dll file on Windows.

To try and clarify, my concern was less about the library itself and more about tests that use the library. I couldn't find any definitive guidance about the different combinations of g{test|mock}_{main} and how their linkage affects the test runner on Windows. Its also easy to overlook when it causes an issue; the test suite still passes, so you'd have to manually inspect the logs to see if tests were actually being run

The main Windows CI job I don't think has FLIGHT tests enabled, so its hard to tell from the CI runs here. When you try locally, are you sure that tests are actually collected and running? If so, then I'm OK with this - I certainly don't want to go down a rabbit hole chasing issues

[alinaliBQ]

@WillAyd Got it, thanks for your clarification. On my branch, I have run ./arrow-flight-sql-test.exe and ./arrow-flight-test.exe on my local Windows MSVC environment and can confirm tests are being collected and run.

I am not very familiar with the combinations of gmock linkage either. Current Windows CI only run on MinGW64 and CLANG64 (that I can find), and I see on Windows CI that flight tests do seem to be run there:

arrow_flight           =   7.06 sec*proc (2 tests)
arrow_flight_sql       =   0.93 sec*proc (1 test)

I think these tests have always been running on the Windows CI because I was able to catch Flight SQL ODBC test failures using CI before the fix of #47434.
#47434 brings an important fix that resolves MSVC Windows specifically. Before #47434, my team running test executables on Windows MSVC locally would also get [ PASSED ] 0 tests., so we also added the RUN_ALL_TESTS as a temporary fix. After #47434, these tests are being run without usages of RUN_ALL_TESTS.
I don't think there is any MSVC Windows CI (for flight at least), and if any issues occur at MSVC Windows we won't be able to catch it using CI, so maybe that's the bigger concern

[WillAyd]

Thanks @alinaliBQ for checking that offline

[alinaliBQ]

No problem!
@kou @lidavidm gentle poke for the PR, please let me know if you have other comments

[kou]

+1

[kou]

Could you remove this comment?
We don't need this now, right?