lidavidm commented on a change in pull request #11507: URL: https://github.com/apache/arrow/pull/11507#discussion_r745680774
########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Ok, sorry, can we avoid using `FlightClientImpl` like this? I don't think this will be maintainable/I think this will be brittle. We can do something like the following: - Make `FlightSqlClient` an abstract base class, with an implementation `template <typename Client> class FlightSqlClientImpl : public FlightSqlClient { ... };` in `client_internal.h`. Then in `client.cc` we can define a factory method that instantiates and returns `FlightSqlClientImpl<FlightClient>` and in `client_test.cc` we can instantiate `FlightSqlClientImpl<MockedFlightClient>`. This way the parameterization on the client impl is explicit and we are not relying on linker magic. - Alternatively, instead of mocking, we can just implement in-process dummy servers to provide the expected reply. This is definitely more code but avoids having to parameterize the client. - Alternatively, we can extract a `FlightClientInterface` or refactor `FlightClient` so it has virtual methods and is directly mockable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
