Skip to content

Commit 605b541

Browse files
HeartLinkedHeartLinked
committed
fix review
1 parent fac5354 commit 605b541

File tree

8 files changed

+43
-140
lines changed

8 files changed

+43
-140
lines changed

‎CMakeLists.txt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,11 @@ else()
6161
set(MSVC_TOOLCHAIN FALSE)
6262
endif()
6363

64+
if(ICEBERG_BUILD_REST_INTEGRATION_TESTS AND WIN32)
65+
set(ICEBERG_BUILD_REST_INTEGRATION_TESTS OFF)
66+
message(WARNING "Cannot build rest integration test on Windows, turning it off.")
67+
endif()
68+
6469
include(CMakeParseArguments)
6570
include(IcebergBuildUtils)
6671
include(IcebergSanitizer)

‎src/iceberg/test/CMakeLists.txt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,10 @@ if(ICEBERG_BUILD_REST)
172172
rest_util_test.cc)
173173

174174
if(ICEBERG_BUILD_REST_INTEGRATION_TESTS)
175-
add_rest_iceberg_test(rest_catalog_integration_test SOURCES rest_catalog_test.cc)
175+
add_rest_iceberg_test(rest_catalog_integration_test
176+
SOURCES
177+
rest_catalog_test.cc
178+
util/cmd_util.cc
179+
util/docker_compose_util.cc)
176180
endif()
177181
endif()

‎src/iceberg/test/meson.build‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,20 @@ if get_option('rest').enabled()
9595
},
9696
}
9797
if get_option('rest_integration_test').enabled()
98-
iceberg_tests += {
99-
'rest_integration_test': {
100-
'sources': files('rest_catalog_test.cc'),
101-
'dependencies': [iceberg_rest_dep],
102-
},
103-
}
98+
if host_machine.system() == 'windows'
99+
warning('Cannot build rest integration test on Windows, skipping.')
100+
else
101+
iceberg_tests += {
102+
'rest_integration_test': {
103+
'sources': files(
104+
'rest_catalog_test.cc',
105+
'util/cmd_util.cc',
106+
'util/docker_compose_util.cc',
107+
),
108+
'dependencies': [iceberg_rest_dep],
109+
},
110+
}
111+
endif
104112
endif
105113
endif
106114

‎src/iceberg/test/util/CMakeLists.txt‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,22 +15,10 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717

18-
set(ICEBERG_TEST_UTIL_SOURCES common_util.cc)
19-
set(ICEBERG_TEST_UTIL_HEADERS common_util.h)
20-
21-
# Platform-specific integration test utilities (Linux-specific)
22-
if(ICEBERG_BUILD_REST_INTEGRATION_TESTS)
23-
list(APPEND ICEBERG_TEST_UTIL_SOURCES cmd_util.cc docker_compose_util.cc)
24-
list(APPEND ICEBERG_TEST_UTIL_HEADERS cmd_util.h docker_compose_util.h)
25-
endif()
26-
27-
add_library(iceberg_test_util STATIC ${ICEBERG_TEST_UTIL_SOURCES})
18+
add_library(iceberg_test_util STATIC common_util.cc)
2819

2920
target_include_directories(iceberg_test_util PUBLIC ${CMAKE_SOURCE_DIR}/src
3021
${CMAKE_BINARY_DIR}/src)
3122

3223
target_link_libraries(iceberg_test_util PUBLIC iceberg_static GTest::gtest
3324
nlohmann_json::nlohmann_json)
34-
35-
install(FILES ${ICEBERG_TEST_UTIL_HEADERS}
36-
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/iceberg/test/util)

‎src/iceberg/test/util/cmd_util.cc‎

Lines changed: 10 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -23,33 +23,33 @@
2323

2424
#include <array>
2525
#include <cstring>
26+
#include <format>
2627
#include <iostream>
2728
#include <print>
29+
#include <stdexcept>
2830

2931
#include <sys/wait.h>
3032

31-
#include "iceberg/result.h"
32-
3333
namespace iceberg {
3434

3535
Command::Command(std::string program) : program_(std::move(program)) {}
3636

37-
Command& Command::arg(std::string a) {
37+
Command& Command::Arg(std::string a) {
3838
args_.push_back(std::move(a));
3939
return *this;
4040
}
4141

42-
Command& Command::args(const std::vector<std::string>& as) {
42+
Command& Command::Args(const std::vector<std::string>& as) {
4343
args_.insert(args_.end(), as.begin(), as.end());
4444
return *this;
4545
}
4646

47-
Command& Command::current_dir(const std::filesystem::path& path) {
47+
Command& Command::CurrentDir(const std::filesystem::path& path) {
4848
cwd_ = path;
4949
return *this;
5050
}
5151

52-
Command& Command::env(const std::string& key, const std::string& val) {
52+
Command& Command::Env(const std::string& key, const std::string& val) {
5353
env_vars_[key] = val;
5454
return *this;
5555
}
@@ -64,7 +64,7 @@ void Command::RunCommand(const std::string& desc) const {
6464

6565
if (pid == -1) {
6666
std::println(stderr, "[ERROR] Fork failed: {}", std::strerror(errno));
67-
throw IOError("Fork failed: {}", std::strerror(errno));
67+
throw std::runtime_error(std::format("Fork failed: {}", std::strerror(errno)));
6868
}
6969

7070
// --- Child Process ---
@@ -102,7 +102,7 @@ void Command::RunCommand(const std::string& desc) const {
102102
int status = 0;
103103
if (waitpid(pid, &status, 0) == -1) {
104104
std::println(stderr, "[ERROR] waitpid failed: {}", std::strerror(errno));
105-
throw IOError("waitpid failed: {}", std::strerror(errno));
105+
throw std::runtime_error(std::format("waitpid failed: {}", std::strerror(errno)));
106106
}
107107

108108
int exit_code = -1;
@@ -117,94 +117,8 @@ void Command::RunCommand(const std::string& desc) const {
117117
return;
118118
} else {
119119
std::println(stderr, "[ERROR] {} failed. Exit code: {}", desc, exit_code);
120-
throw IOError("{} failed with exit code: {}", desc, exit_code);
121-
}
122-
}
123-
124-
std::string Command::GetCommandOutput(const std::string& desc) const {
125-
std::println("[INFO] Starting to capture {}, command: {} ...", desc, program_);
126-
127-
std::cout.flush();
128-
std::cerr.flush();
129-
130-
std::array<int, 2> pipefd{-1, -1};
131-
if (pipe(pipefd.data()) == -1) {
132-
throw IOError("Pipe failed: {}", std::strerror(errno));
133-
}
134-
135-
pid_t pid = fork();
136-
if (pid == -1) {
137-
close(pipefd[0]);
138-
close(pipefd[1]);
139-
throw IOError("Fork failed: {}", std::strerror(errno));
140-
}
141-
142-
// --- Child Process ---
143-
if (pid == 0) {
144-
close(pipefd[0]);
145-
if (dup2(pipefd[1], STDOUT_FILENO) == -1) {
146-
std::println(stderr, "dup2 failed: {}", std::strerror(errno));
147-
_exit(errno);
148-
}
149-
close(pipefd[1]);
150-
151-
if (!cwd_.empty()) {
152-
std::error_code ec;
153-
std::filesystem::current_path(cwd_, ec);
154-
if (ec) {
155-
std::println(stderr, "Failed to change directory to '{}': {}", cwd_.string(),
156-
ec.message());
157-
_exit(126);
158-
}
159-
}
160-
161-
for (const auto& [k, v] : env_vars_) {
162-
setenv(k.c_str(), v.c_str(), 1);
163-
}
164-
165-
std::vector<char*> argv;
166-
argv.reserve(args_.size() + 2);
167-
argv.push_back(const_cast<char*>(program_.c_str()));
168-
for (const auto& arg : args_) {
169-
argv.push_back(const_cast<char*>(arg.c_str()));
170-
}
171-
argv.push_back(nullptr);
172-
173-
execvp(program_.c_str(), argv.data());
174-
// reach here only if execvp fails
175-
std::println(stderr, "execvp failed: {}", std::strerror(errno));
176-
_exit(127);
177-
}
178-
179-
// --- Parent Process ---
180-
close(pipefd[1]);
181-
182-
std::string output;
183-
std::array<char, 4096> buffer;
184-
ssize_t bytes_read;
185-
186-
while ((bytes_read = read(pipefd[0], buffer.data(), buffer.size())) > 0) {
187-
output.append(buffer.data(), static_cast<size_t>(bytes_read));
188-
}
189-
close(pipefd[0]);
190-
191-
int status = 0;
192-
if (waitpid(pid, &status, 0) == -1) {
193-
throw IOError("waitpid failed: {}", std::strerror(errno));
194-
}
195-
196-
int exit_code = -1;
197-
if (WIFEXITED(status)) {
198-
exit_code = WEXITSTATUS(status);
199-
} else if (WIFSIGNALED(status)) {
200-
exit_code = 128 + WTERMSIG(status);
201-
}
202-
203-
if (exit_code == 0) {
204-
return output;
205-
} else {
206-
std::println(stderr, "[ERROR] {} failed. Exit code: {}", desc, exit_code);
207-
throw IOError("{} failed with exit code: {}", desc, exit_code);
120+
throw std::runtime_error(
121+
std::format("{} failed with exit code: {}", desc, exit_code));
208122
}
209123
}
210124

‎src/iceberg/test/util/cmd_util.h‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,31 +35,28 @@ class Command {
3535
explicit Command(std::string program);
3636

3737
/// \brief Add a single argument
38-
Command& arg(std::string a);
38+
Command& Arg(std::string a);
3939

4040
/// \brief Add multiple arguments at once
41-
Command& args(const std::vector<std::string>& as);
41+
Command& Args(const std::vector<std::string>& as);
4242

4343
/// \brief Set the current working directory for the command
44-
Command& current_dir(const std::filesystem::path& path);
44+
Command& CurrentDir(const std::filesystem::path& path);
4545

4646
/// \brief Set an environment variable for the command
47-
Command& env(const std::string& key, const std::string& val);
47+
Command& Env(const std::string& key, const std::string& val);
4848

4949
/// \brief Execute the command and print logs
5050
/// \return A Status indicating success or failure
5151
void RunCommand(const std::string& desc) const;
5252

53-
/// \brief Execute the command and capture its stdout output
54-
/// \return the captured output as a string on success, or an error on failure
55-
std::string GetCommandOutput(const std::string& desc) const;
56-
5753
private:
5854
std::string program_;
5955
std::vector<std::string> args_;
6056
std::filesystem::path cwd_;
6157
std::map<std::string, std::string> env_vars_;
6258

59+
/// \brief Format arguments for logging
6360
std::string fmt_args() const;
6461
};
6562

‎src/iceberg/test/util/docker_compose_util.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,9 @@ void DockerCompose::Down() {
4545
Command DockerCompose::BuildDockerCommand(const std::vector<std::string>& args) const {
4646
Command cmd("docker");
4747
// Set working directory
48-
cmd.current_dir(docker_compose_dir_);
48+
cmd.CurrentDir(docker_compose_dir_);
4949
// Use 'docker compose' subcommand with project name
50-
cmd.arg("compose").arg("-p").arg(project_name_).args(args);
50+
cmd.Arg("compose").Arg("-p").Arg(project_name_).Args(args);
5151
return cmd;
5252
}
5353

‎src/iceberg/test/util/meson.build‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,23 +15,12 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717

18-
iceberg_test_util_sources = files('common_util.cc')
19-
20-
# Platform-specific integration test utilities (Linux-specific)
21-
if get_option('rest_integration_test').enabled()
22-
iceberg_test_util_sources += files('cmd_util.cc', 'docker_compose_util.cc')
23-
install_headers(
24-
['cmd_util.h', 'docker_compose_util.h'],
25-
subdir: 'iceberg/test/util',
26-
)
27-
endif
28-
2918
gtest_dep = dependency('gtest')
3019
nlohmann_json_dep = dependency('nlohmann_json')
3120

3221
iceberg_test_util_lib = static_library(
3322
'iceberg_test_util',
34-
sources: iceberg_test_util_sources,
23+
sources: files('common_util.cc'),
3524
dependencies: [
3625
iceberg_dep,
3726
gtest_dep.partial_dependency(compile_args: true, includes: true),
@@ -44,5 +33,3 @@ iceberg_test_util_dep = declare_dependency(
4433
link_with: iceberg_test_util_lib,
4534
include_directories: include_directories('.'),
4635
)
47-
48-
install_headers(['common_util.h'], subdir: 'iceberg/test/util')

0 commit comments

Comments
 (0)