wgtmac commented on code in PR #361: URL: https://github.com/apache/iceberg-cpp/pull/361#discussion_r2575552682
########## src/iceberg/test/util/CMakeLists.txt: ########## @@ -0,0 +1,36 @@ +# 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. + +set(ICEBERG_TEST_UTIL_SOURCES common_util.cc) +set(ICEBERG_TEST_UTIL_HEADERS common_util.h) + +# Platform-specific integration test utilities (Linux-specific) +if(ICEBERG_BUILD_REST_INTEGRATION_TESTS) + list(APPEND ICEBERG_TEST_UTIL_SOURCES cmd_util.cc docker_compose_util.cc) + list(APPEND ICEBERG_TEST_UTIL_HEADERS cmd_util.h docker_compose_util.h) +endif() + +add_library(iceberg_test_util STATIC ${ICEBERG_TEST_UTIL_SOURCES}) + +target_include_directories(iceberg_test_util PUBLIC ${CMAKE_SOURCE_DIR}/src + ${CMAKE_BINARY_DIR}/src) + +target_link_libraries(iceberg_test_util PUBLIC iceberg_static GTest::gtest + nlohmann_json::nlohmann_json) + +install(FILES ${ICEBERG_TEST_UTIL_HEADERS} Review Comment: We shouldn't install any testing header. ########## src/iceberg/test/util/cmd_util.cc: ########## @@ -0,0 +1,219 @@ +/* + * 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 "iceberg/test/util/cmd_util.h" + +#include <unistd.h> + +#include <array> +#include <cstring> +#include <iostream> +#include <print> + +#include <sys/wait.h> + +#include "iceberg/result.h" + +namespace iceberg { + +Command::Command(std::string program) : program_(std::move(program)) {} + +Command& Command::arg(std::string a) { + args_.push_back(std::move(a)); + return *this; +} + +Command& Command::args(const std::vector<std::string>& as) { + args_.insert(args_.end(), as.begin(), as.end()); + return *this; +} + +Command& Command::current_dir(const std::filesystem::path& path) { + cwd_ = path; + return *this; +} + +Command& Command::env(const std::string& key, const std::string& val) { + env_vars_[key] = val; + return *this; +} + +void Command::RunCommand(const std::string& desc) const { + std::println("[INFO] Starting to {}, command: {} {}", desc, program_, fmt_args()); + + std::cout.flush(); + std::cerr.flush(); + + pid_t pid = fork(); + + if (pid == -1) { + std::println(stderr, "[ERROR] Fork failed: {}", std::strerror(errno)); + throw IOError("Fork failed: {}", std::strerror(errno)); + } + + // --- Child Process --- + if (pid == 0) { + if (!cwd_.empty()) { + std::error_code ec; + std::filesystem::current_path(cwd_, ec); + if (ec) { + std::println(stderr, "Failed to change directory to '{}': {}", cwd_.string(), + ec.message()); + _exit(126); // Command invoked cannot execute + } + } + + for (const auto& [k, v] : env_vars_) { + setenv(k.c_str(), v.c_str(), 1); + } + + std::vector<char*> argv; + argv.reserve(args_.size() + 2); + argv.push_back(const_cast<char*>(program_.c_str())); + + for (const auto& arg : args_) { + argv.push_back(const_cast<char*>(arg.c_str())); + } + argv.push_back(nullptr); + + execvp(program_.c_str(), argv.data()); + + std::println(stderr, "execvp failed: {}", std::strerror(errno)); + _exit(127); + } + + // --- Parent Process --- + int status = 0; + if (waitpid(pid, &status, 0) == -1) { + std::println(stderr, "[ERROR] waitpid failed: {}", std::strerror(errno)); + throw IOError("waitpid failed: {}", std::strerror(errno)); + } + + int exit_code = -1; + if (WIFEXITED(status)) { + exit_code = WEXITSTATUS(status); + } else if (WIFSIGNALED(status)) { + exit_code = 128 + WTERMSIG(status); + } + + if (exit_code == 0) { + std::println("[INFO] {} succeed!", desc); + return; + } else { + std::println(stderr, "[ERROR] {} failed. Exit code: {}", desc, exit_code); + throw IOError("{} failed with exit code: {}", desc, exit_code); + } +} + +std::string Command::GetCommandOutput(const std::string& desc) const { Review Comment: It has a lot of duplicate code with the above one. If it is not used, we can just delete it. Or we can use a more idiomatic approach like below: ```cpp // RAII wrapper for fd class UniqueFd { public: explicit UniqueFd(int fd = -1) : fd_(fd) {} ~UniqueFd() { if (fd_ >= 0) close(fd_); } int get() const { return fd_; } private: int fd_; }; // Wrapper for the forked process class Subprocess { public: struct Options { std::filesystem::path cwd; std::map<std::string, std::string> env; bool capture_stdout = false; }; static Result<Subprocess> Start(const std::string& program, const std::vector<std::string>& args, const Options& opts); Result<int> Wait(); // wait and return exit code Result<std::string> ReadStdout(); // used when capture_stdout is true private: pid_t pid_; UniqueFd fd_; }; ``` ########## src/iceberg/test/rest_catalog_test.cc: ########## @@ -19,103 +19,206 @@ #include "iceberg/catalog/rest/rest_catalog.h" +#include <unistd.h> + +#include <chrono> +#include <memory> +#include <print> #include <string> +#include <thread> #include <unordered_map> +#include <arpa/inet.h> #include <gmock/gmock.h> #include <gtest/gtest.h> +#include <netinet/in.h> +#include <sys/socket.h> #include "iceberg/catalog/rest/catalog_properties.h" +#include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/test/matchers.h" +#include "iceberg/test/util/docker_compose_util.h" namespace iceberg::rest { -// Test fixture for REST catalog tests, This assumes you have a local REST catalog service -// running Default configuration: http://localhost:8181. -class RestCatalogTest : public ::testing::Test { +namespace { + +constexpr uint16_t kRestCatalogPort = 8181; +constexpr int kMaxRetries = 60; // Wait up to 60 seconds +constexpr int kRetryDelayMs = 1000; + +constexpr std::string_view kDockerProjectName = "iceberg-rest-catalog-service"; +constexpr std::string_view kCatalogName = "test_catalog"; +constexpr std::string_view kWarehouseName = "default"; +constexpr std::string_view kLocalhostUri = "http://localhost";; + +/// \brief Check if a localhost port is ready to accept connections +/// \param port Port number to check +/// \return true if the port is accessible on localhost, false otherwise +bool CheckServiceReady(uint16_t port) { + int sock = socket(AF_INET, SOCK_STREAM, 0); + if (sock < 0) { + return false; + } + + struct timeval timeout{ + .tv_sec = 1, + .tv_usec = 0, + }; + setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)); + + sockaddr_in addr{ + .sin_family = AF_INET, + .sin_port = htons(port), + .sin_addr = {.s_addr = htonl(INADDR_LOOPBACK)} // 127.0.0.1 + }; + bool result = + (connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)) == 0); + close(sock); + return result; +} + +} // namespace + +/// \brief Integration test fixture for REST catalog with automatic Docker Compose setup。 +class RestCatalogIntegrationTest : public ::testing::Test { protected: - void SetUp() override { - // Default configuration for local testing - // You can override this with environment variables if needed - const char* uri_env = std::getenv("ICEBERG_REST_URI"); - const char* warehouse_env = std::getenv("ICEBERG_REST_WAREHOUSE"); + static void SetUpTestSuite() { + std::string project_name{kDockerProjectName}; + std::filesystem::path resources_dir = + std::filesystem::path(__FILE__).parent_path() / "resources"; + + // Create and start DockerCompose + docker_compose_ = std::make_unique<DockerCompose>(project_name, resources_dir); + docker_compose_->Up(); + + // Wait for REST catalog to be ready on localhost + std::println("[INFO] Waiting for REST catalog to be ready at localhost:{}...", + kRestCatalogPort); + for (int i = 0; i < kMaxRetries; ++i) { + if (CheckServiceReady(kRestCatalogPort)) { + std::println("[INFO] REST catalog is ready!"); + return; + } + std::println( + "[INFO] Waiting for 1s for REST catalog to be ready... (attempt {}/{})", i + 1, + kMaxRetries); + std::this_thread::sleep_for(std::chrono::milliseconds(kRetryDelayMs)); + } + throw RestError("REST catalog failed to start within {} seconds", kMaxRetries); + } - std::string uri = uri_env ? uri_env : "http://localhost:8181";; - std::string warehouse = warehouse_env ? warehouse_env : "default"; + static void TearDownTestSuite() { docker_compose_.reset(); } + void SetUp() override { config_ = RestCatalogProperties::default_properties(); - config_->Set(RestCatalogProperties::kUri, uri) - .Set(RestCatalogProperties::kName, std::string("test_catalog")) - .Set(RestCatalogProperties::kWarehouse, warehouse); + config_ + ->Set(RestCatalogProperties::kUri, + std::format("{}:{}", kLocalhostUri, kRestCatalogPort)) + .Set(RestCatalogProperties::kName, std::string(kCatalogName)) + .Set(RestCatalogProperties::kWarehouse, std::string(kWarehouseName)); } void TearDown() override {} + /// \brief Helper function to create a REST catalog instance + Result<std::unique_ptr<RestCatalog>> CreateCatalog() { + return RestCatalog::Make(*config_); + } + + static inline std::unique_ptr<DockerCompose> docker_compose_; std::unique_ptr<RestCatalogProperties> config_; }; -TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) { - auto catalog_result = RestCatalog::Make(*config_); - EXPECT_THAT(catalog_result, IsOk()); +TEST_F(RestCatalogIntegrationTest, MakeCatalogSuccess) { + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); - if (catalog_result.has_value()) { - auto& catalog = catalog_result.value(); - EXPECT_EQ(catalog->name(), "test_catalog"); - } + auto& catalog = catalog_result.value(); + EXPECT_EQ(catalog->name(), kCatalogName); } -TEST_F(RestCatalogTest, DISABLED_MakeCatalogEmptyUri) { - auto invalid_config = RestCatalogProperties::default_properties(); - invalid_config->Set(RestCatalogProperties::kUri, std::string("")); +TEST_F(RestCatalogIntegrationTest, ListNamespaces) { + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); - auto catalog_result = RestCatalog::Make(*invalid_config); - EXPECT_THAT(catalog_result, IsError(ErrorKind::kInvalidArgument)); - EXPECT_THAT(catalog_result, HasErrorMessage("uri")); + Namespace root{.levels = {}}; + auto result = catalog->ListNamespaces(root); + EXPECT_THAT(result, IsOk()); } -TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) { - auto custom_config = RestCatalogProperties::default_properties(); - custom_config - ->Set(RestCatalogProperties::kUri, config_->Get(RestCatalogProperties::kUri)) - .Set(RestCatalogProperties::kName, config_->Get(RestCatalogProperties::kName)) - .Set(RestCatalogProperties::kWarehouse, - config_->Get(RestCatalogProperties::kWarehouse)) - .Set(RestCatalogProperties::Entry<std::string>{"custom_prop", ""}, - std::string("custom_value")) - .Set(RestCatalogProperties::Entry<std::string>{"timeout", ""}, - std::string("30000")); - - auto catalog_result = RestCatalog::Make(*custom_config); - EXPECT_THAT(catalog_result, IsOk()); +TEST_F(RestCatalogIntegrationTest, DISABLED_GetNonExistentNamespace) { + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); + + Namespace ns{.levels = {"test_get_non_existent_namespace"}}; + auto result = catalog->GetNamespaceProperties(ns); + + EXPECT_THAT(result, HasErrorMessage("does not exist")); } -TEST_F(RestCatalogTest, DISABLED_ListNamespaces) { - auto catalog_result = RestCatalog::Make(*config_); +TEST_F(RestCatalogIntegrationTest, DISABLED_CreateAndDropNamespace) { Review Comment: Why this and below are still disabled? ########## src/iceberg/test/util/cmd_util.cc: ########## @@ -0,0 +1,219 @@ +/* + * 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 "iceberg/test/util/cmd_util.h" + +#include <unistd.h> + +#include <array> +#include <cstring> +#include <iostream> +#include <print> + +#include <sys/wait.h> + +#include "iceberg/result.h" + +namespace iceberg { + +Command::Command(std::string program) : program_(std::move(program)) {} + +Command& Command::arg(std::string a) { + args_.push_back(std::move(a)); + return *this; +} + +Command& Command::args(const std::vector<std::string>& as) { + args_.insert(args_.end(), as.begin(), as.end()); + return *this; +} + +Command& Command::current_dir(const std::filesystem::path& path) { + cwd_ = path; + return *this; +} + +Command& Command::env(const std::string& key, const std::string& val) { + env_vars_[key] = val; + return *this; +} + +void Command::RunCommand(const std::string& desc) const { + std::println("[INFO] Starting to {}, command: {} {}", desc, program_, fmt_args()); + + std::cout.flush(); + std::cerr.flush(); + + pid_t pid = fork(); + + if (pid == -1) { + std::println(stderr, "[ERROR] Fork failed: {}", std::strerror(errno)); + throw IOError("Fork failed: {}", std::strerror(errno)); Review Comment: Where does this `IOError` come from? I remember that our `IOError` is not an exception. ########## src/iceberg/test/util/meson.build: ########## @@ -0,0 +1,48 @@ +# 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. + +iceberg_test_util_sources = files('common_util.cc') + +# Platform-specific integration test utilities (Linux-specific) +if get_option('rest_integration_test').enabled() + iceberg_test_util_sources += files('cmd_util.cc', 'docker_compose_util.cc') + install_headers( Review Comment: Same here. Please do not install these headers. ########## src/iceberg/test/util/CMakeLists.txt: ########## @@ -0,0 +1,36 @@ +# 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. + +set(ICEBERG_TEST_UTIL_SOURCES common_util.cc) +set(ICEBERG_TEST_UTIL_HEADERS common_util.h) + +# Platform-specific integration test utilities (Linux-specific) +if(ICEBERG_BUILD_REST_INTEGRATION_TESTS) + list(APPEND ICEBERG_TEST_UTIL_SOURCES cmd_util.cc docker_compose_util.cc) + list(APPEND ICEBERG_TEST_UTIL_HEADERS cmd_util.h docker_compose_util.h) +endif() + +add_library(iceberg_test_util STATIC ${ICEBERG_TEST_UTIL_SOURCES}) Review Comment: I don't think it is a good idea to add a new testing library for this. If `ICEBERG_BUILD_REST_INTEGRATION_TESTS` is on, all test executables are linked with symbols that are only useful for rest integration tests. A better and simpler approach is just to add these files to the executable of integration test. ########## CMakeLists.txt: ########## @@ -42,6 +42,7 @@ option(ICEBERG_BUILD_SHARED "Build shared library" OFF) option(ICEBERG_BUILD_TESTS "Build tests" ON) option(ICEBERG_BUILD_BUNDLE "Build the battery included library" ON) option(ICEBERG_BUILD_REST "Build rest catalog client" ON) +option(ICEBERG_BUILD_REST_INTEGRATION_TESTS "Build rest catalog integration tests" OFF) Review Comment: We need to reset it to off on Windows. Something like below ```cmake if(ICEBERG_BUILD_REST_INTEGRATION_TESTS AND WIN32) set(ICEBERG_BUILD_REST_INTEGRATION_TESTS OFF) message(WARNING "Cannot build rest integration test on Windows, turning it off.") endlf() ``` ########## src/iceberg/test/util/cmd_util.h: ########## @@ -0,0 +1,66 @@ +/* + * 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. + */ + +#pragma once + +#include <filesystem> +#include <map> +#include <string> +#include <vector> + +/// \file iceberg/test/util/cmd_util.h +/// Utilities for building and executing shell commands in tests. + +namespace iceberg { + +/// \brief A shell command builder and executor for tests. +class Command { + public: + explicit Command(std::string program); + + /// \brief Add a single argument + Command& arg(std::string a); Review Comment: Please follow the naming style to use capitalized initials. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
