Skip to content

GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available #50215

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

Open
raulcd wants to merge 6 commits into
base: main
Choose a base branch
from
Open

GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available #50215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43e19e4
GH-40410: [C++] Re-enable S3 tests for macOS
raulcd Jun 18, 2026
1c84deb
Add minio installation to path so tests are not skipped
raulcd Jun 18, 2026
bdbda3f
Skip only arrow-s3fs-tests and arrow-s3fs-module-tests that require m…
raulcd Jun 18, 2026
4faccf6
Fix remnant from previous excludes
raulcd Jun 18, 2026
eff7ef0
Take into account possible external s3-compatible service configured
raulcd Jun 18, 2026
36b2b4a
Move flags to single std::optional<bool>
raulcd Jun 19, 2026
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
2 changes: 2 additions & 0 deletions .github/workflows/cpp.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ jobs:
- name: Test
shell: bash
run: |
# MinIO is installed on ${ARROW_HOME}/bin
export PATH="${ARROW_HOME}/bin:${PATH}"
sudo sysctl -w kern.coredump=1
sudo sysctl -w kern.corefile=/tmp/core.%N.%P
ulimit -c unlimited # must enable within the same shell
Expand Down
4 changes: 1 addition & 3 deletions .github/workflows/cpp_extra.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,8 @@ jobs:
ARROW_TEST_DATA: ${{ github.workspace }}/testing/data
PARQUET_TEST_DATA: ${{ github.workspace }}/cpp/submodules/parquet-testing/data
run: |
# MinIO is required
exclude_tests="arrow-s3fs-test"
# unstable
exclude_tests="${exclude_tests}|arrow-acero-asof-join-node-test"
exclude_tests="arrow-acero-asof-join-node-test"
exclude_tests="${exclude_tests}|arrow-acero-hash-join-node-test"
ctest \
--exclude-regex "${exclude_tests}" \
Expand Down
5 changes: 0 additions & 5 deletions ci/scripts/cpp_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,12 @@ fi
if ! type storage-testbench >/dev/null 2>&1; then
exclude_tests+=("arrow-gcsfs-test")
fi
if ! type minio >/dev/null 2>&1; then
exclude_tests+=("arrow-s3fs-test")
fi
case "$(uname)" in
Linux)
n_jobs=$(nproc)
;;
Darwin)
n_jobs=$(sysctl -n hw.ncpu)
# TODO: https://github.com/apache/arrow/issues/40410
exclude_tests+=("arrow-s3fs-test")
;;
MINGW*)
n_jobs=${NUMBER_OF_PROCESSORS:-1}
Expand Down
13 changes: 13 additions & 0 deletions cpp/src/arrow/filesystem/s3_test_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,19 @@ void MinioTestEnvironment::SetUp() {
MakeReadaheadGenerator(std::move(impl_->server_generator_), pool->GetCapacity());
}

bool MinioTestEnvironment::IsAvailable() {
if (!available_.has_value()) {
// If external S3-compatible service is configured there's no need to check for Minio
available_ = std::getenv(kEnvConnectString) && std::getenv(kEnvAccessKey) &&
std::getenv(kEnvSecretKey);
if (!available_.value()) {
::arrow::util::Process process;
available_ = process.SetExecutable(kMinioExecutableName).ok();
}
}
return available_.value();
}

Result<std::shared_ptr<MinioTestServer>> MinioTestEnvironment::GetOneServer() {
return impl_->server_generator_().result();
}
Expand Down
6 changes: 6 additions & 0 deletions cpp/src/arrow/filesystem/s3_test_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#pragma once

#include <memory>
#include <optional>
#include <string>
#include <utility>

Expand Down Expand Up @@ -74,6 +75,11 @@ class MinioTestEnvironment : public ::testing::Environment {

Result<std::shared_ptr<MinioTestServer>> GetOneServer();

bool IsAvailable();

private:
std::optional<bool> available_;

protected:
struct Impl;
std::unique_ptr<Impl> impl_;
Expand Down
33 changes: 20 additions & 13 deletions cpp/src/arrow/filesystem/s3fs_module_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,17 @@ MinioTestEnvironment* GetMinioEnv() {
return ::arrow::internal::checked_cast<MinioTestEnvironment*>(minio_env);
}

class S3ModuleTest : public ::testing::Test {
protected:
void SetUp() override {
if (!GetMinioEnv()->IsAvailable()) {

@pitrou pitrou Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This also means that any CI configuration regression where Minio is not on PATH would not be caught by the test suite. We can perhaps live with that, but it makes our setup a bit more fragile than previously.

@raulcd raulcd Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

isn't this the same as today? We have a check on cpp_test.sh where if MinIO is not on path we exclude the tests and any CI configuration regression where MinIO is removed from the PATH would not be caught today:

if ! type minio >/dev/null 2>&1; then
  exclude_tests+=("arrow-s3fs-test")
fi

This check would still be required if we split arrow-s3fs-test and arrow-s3fs-minio-test unless we manually enforce on CI jobs when tests have to be skipped, something like a CI flag.

I started working on a split but it's slightly invasive. Given we probably should migrate MinIO, I'd rather do that on a different PR.

@pitrou pitrou Jun 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't this the same as today? We have a check on cpp_test.sh where if MinIO is not on path we exclude the tests and any CI configuration regression where MinIO is removed from the PATH would not be caught today:

Oops! Ok, then nevermind :)

GTEST_SKIP() << "Minio executable not found, skipping tests";
}
ASSERT_OK_AND_ASSIGN(minio_, GetMinioEnv()->GetOneServer());
}
std::shared_ptr<MinioTestServer> minio_;
};

class RegistrationTestEnvironment : public ::testing::Environment {
public:
void SetUp() override {
Expand All @@ -68,12 +79,10 @@ class RegistrationTestEnvironment : public ::testing::Environment {

auto* lib_env = ::testing::AddGlobalTestEnvironment(new RegistrationTestEnvironment);

TEST(S3Test, FromUri) {
ASSERT_OK_AND_ASSIGN(auto minio, GetMinioEnv()->GetOneServer());

TEST_F(S3ModuleTest, FromUri) {
std::string path;
ASSERT_OK_AND_ASSIGN(auto fs, FileSystemFromUri("s3://" + minio->access_key() + ":" +
minio->secret_key() +
ASSERT_OK_AND_ASSIGN(auto fs, FileSystemFromUri("s3://" + minio_->access_key() + ":" +
minio_->secret_key() +
"@bucket/somedir/subdir/subfile",
&path));

Expand All @@ -83,12 +92,11 @@ TEST(S3Test, FromUri) {
"&allow_bucket_creation=0&allow_bucket_deletion=0");
}

TEST(S3Test, FromUriAndOptionsCredentials) {
ASSERT_OK_AND_ASSIGN(auto minio, GetMinioEnv()->GetOneServer());
TEST_F(S3ModuleTest, FromUriAndOptionsCredentials) {
std::string path;
FileSystemFactoryOptions options{
{"access_key", std::string(minio->access_key())},
{"secret_key", std::string(minio->secret_key())},
{"access_key", std::string(minio_->access_key())},
{"secret_key", std::string(minio_->secret_key())},
};
// Credentials supplied via options, NOT in the URI.
ASSERT_OK_AND_ASSIGN(
Expand All @@ -111,11 +119,10 @@ class NoopRetryStrategy : public S3RetryStrategy {
};
} // namespace

TEST(S3Test, FromUriAndOptionsRetryStrategy) {
ASSERT_OK_AND_ASSIGN(auto minio, GetMinioEnv()->GetOneServer());
TEST_F(S3ModuleTest, FromUriAndOptionsRetryStrategy) {
FileSystemFactoryOptions options{
{"access_key", std::string(minio->access_key())},
{"secret_key", std::string(minio->secret_key())},
{"access_key", std::string(minio_->access_key())},
{"secret_key", std::string(minio_->secret_key())},
{"retry_strategy",
std::shared_ptr<S3RetryStrategy>(std::make_shared<NoopRetryStrategy>())},
};
Expand Down
5 changes: 5 additions & 0 deletions cpp/src/arrow/filesystem/s3fs_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ class S3TestMixin : public AwsTestMixin {
public:
void SetUp() override {
AwsTestMixin::SetUp();
if (!GetMinioEnv(enable_tls_)->IsAvailable()) {
GTEST_SKIP() << "Minio executable not found, skipping tests";
}

// Starting the server may fail, for example if the generated port number
// was "stolen" by another process. Run a dummy S3 operation to make sure it
Expand Down Expand Up @@ -624,6 +627,7 @@ class TestS3FS : public S3TestMixin {
public:
void SetUp() override {
S3TestMixin::SetUp();
if (IsSkipped()) return;
// Most tests will create buckets
options_.allow_bucket_creation = true;
options_.allow_bucket_deletion = true;
Expand Down Expand Up @@ -1777,6 +1781,7 @@ class TestS3FSGeneric : public S3TestMixin, public GenericFileSystemTest {
public:
void SetUp() override {
S3TestMixin::SetUp();
if (IsSkipped()) return;
// Set up test bucket
{
Aws::S3::Model::CreateBucketRequest req;
Expand Down
Loading