From 22bc30a166dc34631d08e1fe24461b93efa70671 Mon Sep 17 00:00:00 2001 From: David Li Date: Mon, 16 May 2022 16:51:34 -0400 Subject: [PATCH 1/2] ARROW-16588: [C++][FlightRPC] Don't subclass GTest in test helpers Also, don't link every Arrow library to UCX when enabled. --- cpp/CMakeLists.txt | 5 ---- cpp/src/arrow/flight/flight_test.cc | 28 ++++++++++++----- cpp/src/arrow/flight/test_definitions.cc | 28 ++++++++--------- cpp/src/arrow/flight/test_definitions.h | 30 +++++++++---------- .../ucx/flight_transport_ucx_test.cc | 28 ++++++++++++----- 5 files changed, 71 insertions(+), 48 deletions(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index eb97f3ad9d7..23e0ba311f3 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -827,11 +827,6 @@ if(ARROW_USE_XSIMD) list(APPEND ARROW_STATIC_LINK_LIBS xsimd) endif() -if(ARROW_WITH_UCX) - list(APPEND ARROW_LINK_LIBS ucx::ucx) - list(APPEND ARROW_STATIC_LINK_LIBS ucx::ucx) -endif() - add_custom_target(arrow_dependencies) add_custom_target(arrow_benchmark_dependencies) add_custom_target(arrow_test_dependencies) diff --git a/cpp/src/arrow/flight/flight_test.cc b/cpp/src/arrow/flight/flight_test.cc index 5f86fc32928..a7c79a6dc5b 100644 --- a/cpp/src/arrow/flight/flight_test.cc +++ b/cpp/src/arrow/flight/flight_test.cc @@ -73,45 +73,59 @@ const char kAuthHeader[] = "authorization"; //------------------------------------------------------------ // Common transport tests -class GrpcConnectivityTest : public ConnectivityTest { +class GrpcConnectivityTest : public ConnectivityTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_CONNECTIVITY(GrpcConnectivityTest); -class GrpcDataTest : public DataTest { +class GrpcDataTest : public DataTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_DATA(GrpcDataTest); -class GrpcDoPutTest : public DoPutTest { +class GrpcDoPutTest : public DoPutTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_DO_PUT(GrpcDoPutTest); -class GrpcAppMetadataTest : public AppMetadataTest { +class GrpcAppMetadataTest : public AppMetadataTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_APP_METADATA(GrpcAppMetadataTest); -class GrpcIpcOptionsTest : public IpcOptionsTest { +class GrpcIpcOptionsTest : public IpcOptionsTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_IPC_OPTIONS(GrpcIpcOptionsTest); -class GrpcCudaDataTest : public CudaDataTest { +class GrpcCudaDataTest : public CudaDataTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_CUDA_DATA(GrpcCudaDataTest); -class GrpcErrorHandlingTest : public ErrorHandlingTest { +class GrpcErrorHandlingTest : public ErrorHandlingTest, public ::testing::Test { protected: std::string transport() const override { return "grpc"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_ERROR_HANDLING(GrpcErrorHandlingTest); diff --git a/cpp/src/arrow/flight/test_definitions.cc b/cpp/src/arrow/flight/test_definitions.cc index fbe23b789d4..61873ba36da 100644 --- a/cpp/src/arrow/flight/test_definitions.cc +++ b/cpp/src/arrow/flight/test_definitions.cc @@ -114,7 +114,7 @@ void ConnectivityTest::TestBrokenConnection() { //------------------------------------------------------------ // Tests of data plane methods -void DataTest::SetUp() { +void DataTest::SetUpTest() { server_ = ExampleTestServer(); ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "127.0.0.1", 0)); @@ -123,7 +123,7 @@ void DataTest::SetUp() { ASSERT_OK(ConnectClient()); } -void DataTest::TearDown() { +void DataTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); } @@ -637,14 +637,14 @@ class DoPutTestServer : public FlightServerBase { friend class DoPutTest; }; -void DoPutTest::SetUp() { +void DoPutTest::SetUpTest() { ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "127.0.0.1", 0)); ASSERT_OK(MakeServer( location, &server_, &client_, [](FlightServerOptions* options) { return Status::OK(); }, [](FlightClientOptions* options) { return Status::OK(); })); } -void DoPutTest::TearDown() { +void DoPutTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); reinterpret_cast(server_.get())->batches_.clear(); @@ -865,14 +865,14 @@ Status AppMetadataTestServer::DoPut(const ServerCallContext& context, return Status::OK(); } -void AppMetadataTest::SetUp() { +void AppMetadataTest::SetUpTest() { ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "127.0.0.1", 0)); ASSERT_OK(MakeServer( location, &server_, &client_, [](FlightServerOptions* options) { return Status::OK(); }, [](FlightClientOptions* options) { return Status::OK(); })); } -void AppMetadataTest::TearDown() { +void AppMetadataTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); } @@ -1044,14 +1044,14 @@ class IpcOptionsTestServer : public FlightServerBase { } }; -void IpcOptionsTest::SetUp() { +void IpcOptionsTest::SetUpTest() { ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "127.0.0.1", 0)); ASSERT_OK(MakeServer( location, &server_, &client_, [](FlightServerOptions* options) { return Status::OK(); }, [](FlightClientOptions* options) { return Status::OK(); })); } -void IpcOptionsTest::TearDown() { +void IpcOptionsTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); } @@ -1232,7 +1232,7 @@ class CudaDataTest::Impl { std::shared_ptr context; }; -void CudaDataTest::SetUp() { +void CudaDataTest::SetUpTest() { ASSERT_OK_AND_ASSIGN(auto manager, cuda::CudaDeviceManager::Instance()); ASSERT_OK_AND_ASSIGN(auto device, manager->GetDevice(0)); ASSERT_OK_AND_ASSIGN(auto context, device->GetContext()); @@ -1251,7 +1251,7 @@ void CudaDataTest::SetUp() { [](FlightClientOptions* options) { return Status::OK(); }, impl_->device, impl_->context)); } -void CudaDataTest::TearDown() { +void CudaDataTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); } @@ -1353,8 +1353,8 @@ void CudaDataTest::TestDoExchange() { #else -void CudaDataTest::SetUp() {} -void CudaDataTest::TearDown() {} +void CudaDataTest::SetUpTest() {} +void CudaDataTest::TearDownTest() {} void CudaDataTest::TestDoGet() { GTEST_SKIP() << "Arrow was built without ARROW_CUDA"; } void CudaDataTest::TestDoPut() { GTEST_SKIP() << "Arrow was built without ARROW_CUDA"; } void CudaDataTest::TestDoExchange() { @@ -1440,14 +1440,14 @@ class ErrorHandlingTestServer : public FlightServerBase { }; } // namespace -void ErrorHandlingTest::SetUp() { +void ErrorHandlingTest::SetUpTest() { ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "127.0.0.1", 0)); ASSERT_OK(MakeServer( location, &server_, &client_, [](FlightServerOptions* options) { return Status::OK(); }, [](FlightClientOptions* options) { return Status::OK(); })); } -void ErrorHandlingTest::TearDown() { +void ErrorHandlingTest::TearDownTest() { ASSERT_OK(client_->Close()); ASSERT_OK(server_->Shutdown()); } diff --git a/cpp/src/arrow/flight/test_definitions.h b/cpp/src/arrow/flight/test_definitions.h index 464631455dd..aa0d07857be 100644 --- a/cpp/src/arrow/flight/test_definitions.h +++ b/cpp/src/arrow/flight/test_definitions.h @@ -24,8 +24,6 @@ #pragma once -#include - #include #include #include @@ -39,9 +37,11 @@ namespace arrow { namespace flight { -class ARROW_FLIGHT_EXPORT FlightTest : public testing::Test { +class ARROW_FLIGHT_EXPORT FlightTest { protected: virtual std::string transport() const = 0; + virtual void SetUpTest() {} + virtual void TearDownTest() {} }; /// Common tests of startup/shutdown @@ -67,8 +67,8 @@ class ARROW_FLIGHT_EXPORT ConnectivityTest : public FlightTest { /// Common tests of data plane methods class ARROW_FLIGHT_EXPORT DataTest : public FlightTest { public: - void SetUp(); - void TearDown(); + void SetUpTest() override; + void TearDownTest() override; Status ConnectClient(); // Test methods @@ -124,8 +124,8 @@ class ARROW_FLIGHT_EXPORT DataTest : public FlightTest { /// \brief Specific tests of DoPut. class ARROW_FLIGHT_EXPORT DoPutTest : public FlightTest { public: - void SetUp(); - void TearDown(); + void SetUpTest() override; + void TearDownTest() override; void CheckBatches(const FlightDescriptor& expected_descriptor, const RecordBatchVector& expected_batches); void CheckDoPut(const FlightDescriptor& descr, const std::shared_ptr& schema, @@ -171,8 +171,8 @@ class ARROW_FLIGHT_EXPORT AppMetadataTestServer : public FlightServerBase { /// \brief Tests of app_metadata in data plane methods. class ARROW_FLIGHT_EXPORT AppMetadataTest : public FlightTest { public: - void SetUp(); - void TearDown(); + void SetUpTest() override; + void TearDownTest() override; // Test methods void TestDoGet(); @@ -198,8 +198,8 @@ class ARROW_FLIGHT_EXPORT AppMetadataTest : public FlightTest { /// \brief Tests of IPC options in data plane methods. class ARROW_FLIGHT_EXPORT IpcOptionsTest : public FlightTest { public: - void SetUp(); - void TearDown(); + void SetUpTest() override; + void TearDownTest() override; // Test methods void TestDoGetReadOptions(); @@ -233,8 +233,8 @@ class ARROW_FLIGHT_EXPORT IpcOptionsTest : public FlightTest { /// If not built with ARROW_CUDA, tests are no-ops. class ARROW_FLIGHT_EXPORT CudaDataTest : public FlightTest { public: - void SetUp() override; - void TearDown() override; + void SetUpTest() override; + void TearDownTest() override; // Test methods void TestDoGet(); @@ -258,8 +258,8 @@ class ARROW_FLIGHT_EXPORT CudaDataTest : public FlightTest { /// \brief Tests of error handling. class ARROW_FLIGHT_EXPORT ErrorHandlingTest : public FlightTest { public: - void SetUp() override; - void TearDown() override; + void SetUpTest() override; + void TearDownTest() override; // Test methods void TestGetFlightInfo(); diff --git a/cpp/src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc b/cpp/src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc index a29d498d0be..1e2599ff119 100644 --- a/cpp/src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc +++ b/cpp/src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc @@ -50,45 +50,59 @@ testing::Environment* const kUcxEnvironment = //------------------------------------------------------------ // Common transport tests -class UcxConnectivityTest : public ConnectivityTest { +class UcxConnectivityTest : public ConnectivityTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_CONNECTIVITY(UcxConnectivityTest); -class UcxDataTest : public DataTest { +class UcxDataTest : public DataTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_DATA(UcxDataTest); -class UcxDoPutTest : public DoPutTest { +class UcxDoPutTest : public DoPutTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_DO_PUT(UcxDoPutTest); -class UcxAppMetadataTest : public AppMetadataTest { +class UcxAppMetadataTest : public AppMetadataTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_APP_METADATA(UcxAppMetadataTest); -class UcxIpcOptionsTest : public IpcOptionsTest { +class UcxIpcOptionsTest : public IpcOptionsTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_IPC_OPTIONS(UcxIpcOptionsTest); -class UcxCudaDataTest : public CudaDataTest { +class UcxCudaDataTest : public CudaDataTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_CUDA_DATA(UcxCudaDataTest); -class UcxErrorHandlingTest : public ErrorHandlingTest { +class UcxErrorHandlingTest : public ErrorHandlingTest, public ::testing::Test { protected: std::string transport() const override { return "ucx"; } + void SetUp() override { SetUpTest(); } + void TearDown() override { TearDownTest(); } }; ARROW_FLIGHT_TEST_ERROR_HANDLING(UcxErrorHandlingTest); From e70fea53392a70441f44d2c560b91e3a8cdf607d Mon Sep 17 00:00:00 2001 From: David Li Date: Tue, 17 May 2022 09:25:27 -0400 Subject: [PATCH 2/2] Fix bundled build --- cpp/src/arrow/flight/transport/ucx/CMakeLists.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt b/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt index 71392ec0af4..d682ead6336 100644 --- a/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt +++ b/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt @@ -53,6 +53,7 @@ if(ARROW_BUILD_TESTS) arrow_flight_static arrow_flight_testing_static arrow_flight_transport_ucx_static + ucx::ucx ${ARROW_TEST_LINK_LIBS}) else() set(ARROW_FLIGHT_UCX_TEST_LINK_LIBS @@ -60,6 +61,7 @@ if(ARROW_BUILD_TESTS) arrow_flight_shared arrow_flight_testing_shared arrow_flight_transport_ucx_shared + ucx::ucx ${ARROW_TEST_LINK_LIBS}) endif() add_arrow_test(flight_transport_ucx_test