Skip to content

ARROW-16588: [C++][FlightRPC] Don't subclass GTest in test helpers #13169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lidavidm wants to merge 2 commits into from
Closed

ARROW-16588: [C++][FlightRPC] Don't subclass GTest in test helpers #13169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22bc30a
ARROW-16588: [C++][FlightRPC] Don't subclass GTest in test helpers
lidavidm May 16, 2022
e70fea5
Fix bundled build
lidavidm May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Comment on lines -830 to -834
Copy link
Contributor

@cyb70289 cyb70289 May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it causes bundled ucx build error.
I'm not pretty sure of what this two lines does. Is it possible to include the headers somewhere else? @kou

In file included from ../src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc:33:
../src/arrow/flight/transport/ucx/ucx_internal.h:27:10: fatal error: 'ucp/api/ucp.h' file not found
#include <ucp/api/ucp.h>
         ^~~~~~~~~~~~~~~
1 error generated.

Copy link
Member

@kou kou May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try the following patch?

diff --git a/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt b/cpp/src/arrow/flight/transport/ucx/CMakeLists.txt
index 71392ec0af..d682ead633 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

Copy link
Contributor

@cyb70289 cyb70289 May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. Thanks @kou!

Copy link
Member Author

@lidavidm lidavidm May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Thanks for the fix!

add_custom_target(arrow_dependencies)
add_custom_target(arrow_benchmark_dependencies)
add_custom_target(arrow_test_dependencies)
Expand Down
28 changes: 21 additions & 7 deletions cpp/src/arrow/flight/flight_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
28 changes: 14 additions & 14 deletions cpp/src/arrow/flight/test_definitions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -123,7 +123,7 @@ void DataTest::SetUp() {

ASSERT_OK(ConnectClient());
}
void DataTest::TearDown() {
void DataTest::TearDownTest() {
ASSERT_OK(client_->Close());
ASSERT_OK(server_->Shutdown());
}
Expand Down Expand Up @@ -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<DoPutTestServer>(
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<DoPutTestServer*>(server_.get())->batches_.clear();
Expand Down Expand Up @@ -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<AppMetadataTestServer>(
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());
}
Expand Down Expand Up @@ -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<IpcOptionsTestServer>(
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());
}
Expand Down Expand Up @@ -1232,7 +1232,7 @@ class CudaDataTest::Impl {
std::shared_ptr<cuda::CudaContext> 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());
Expand All @@ -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());
}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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<ErrorHandlingTestServer>(
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());
}
Expand Down
30 changes: 15 additions & 15 deletions cpp/src/arrow/flight/test_definitions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@

#pragma once

#include <gtest/gtest.h>

#include <functional>
#include <memory>
#include <string>
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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>& schema,
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/flight/transport/ucx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,15 @@ 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
arrow_shared
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
Expand Down
28 changes: 21 additions & 7 deletions cpp/src/arrow/flight/transport/ucx/flight_transport_ucx_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down