You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "raulcd (via GitHub)" <gi...@apache.org> on 2026/06/18 09:11:31 UTC
[PR] GH-40410: [C++] Re-enable S3 tests for macOS [arrow]
raulcd opened a new pull request, #50215:
URL: https://github.com/apache/arrow/pull/50215
### Rationale for this change
There are lots of S3 tests that doesn't require minio which we are not exercising on CI. We should exercise those and only skip the minio ones
### What changes are included in this PR?
TBD
### Are these changes tested?
Yes via CI and existing tests.
### Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3440903266
##########
cpp/src/arrow/filesystem/s3fs_module_test.cc:
##########
@@ -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()) {
Review Comment:
isn't this the same as today? We have a check on `cpp_build.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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3435732786
##########
cpp/src/arrow/filesystem/s3_test_util.cc:
##########
@@ -192,6 +192,15 @@ void MinioTestEnvironment::SetUp() {
MakeReadaheadGenerator(std::move(impl_->server_generator_), pool->GetCapacity());
}
+bool MinioTestEnvironment::IsAvailable() {
+ if (!checked_) {
+ ::arrow::util::Process process;
+ available_ = process.SetExecutable(kMinioExecutableName).ok();
+ checked_ = true;
+ }
+ return available_;
+}
Review Comment:
I didn't know this existed, this was a good catch :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4766973317
CI failures are unrelated:
- `test-conda-cpp-valgrind` has been fixed on main (not related to this PR)
- `test-debian-experimental-cpp-gcc-15` also not related it seems to be failing on nightlies too
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3435986284
##########
cpp/src/arrow/filesystem/s3fs_module_test.cc:
##########
@@ -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()) {
Review Comment:
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.
##########
cpp/src/arrow/filesystem/s3_test_util.h:
##########
@@ -74,6 +74,12 @@ class MinioTestEnvironment : public ::testing::Environment {
Result<std::shared_ptr<MinioTestServer>> GetOneServer();
+ bool IsAvailable();
+
+ private:
+ bool checked_ = false;
+ bool available_ = false;
Review Comment:
Can perhaps make this a single `std::optional<bool>`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4741804186
I can create a new issue instead of reusing the one that prompted to revisit the exclude_tests as to be fair this PR covers more than that one.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3440955761
##########
cpp/src/arrow/filesystem/s3fs_module_test.cc:
##########
@@ -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()) {
Review Comment:
> 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 :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4766369107
Revision: 36b2b4adfc4742dba37b9d51b8296b19ae747284
Submitted crossbow builds: [ursacomputing/crossbow @ actions-ad025a4352](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ad025a4352)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662367/job/82669679454)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663795/job/82669684189)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663243/job/82669682327)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663580/job/82669682957)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663847/job/82669684164)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662423/job/82669679840)|
|test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/27939664155/job/82669685039)|
|test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663649/job/82669683043)|
|test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662339/job/82669679577)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27939664289/job/82669685469)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663442/job/82669682629)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662763/job/82669680856)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662871/job/82669681162)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662298/job/82669679366)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663305/job/82669682101)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/27939664409/job/82669686420)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/27939663368/job/82669682741)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662812/job/82669680673)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/27939662473/job/82669680086)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/27939664234/job/82669685432)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4771617430
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c75b82dd594d60b056ec7539ec2b7e8ba2aaff13.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/82791421226) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4741794181
@kou @pitrou while working on the S3 module split I've realised we were not testing s3fs tests on macOS due to the unconditional `exclude_tests` on Darwin. I have fixed that and validated they pass for macOS (had to also fix the path discovery).
I've also found the current `exclude_tests` approach too broad if MinIO isn't found. We have plenty of tests on `arrow-s3fs-test` that were skipped which don't require MinIO and we are adding more with the `arrow-s3fs-module-test`. I've decided to push a different approach. Add a check on the test fixture to validate whether MinIO is available or not and skip only the tests that require MinIO so the full exclude is not necessary. Let me know what you think.
I've validated this locally both ways: without MinIO the non-MinIO tests run and the MinIO dependant ones are skipped, with MinIO on PATH the full suite runs and passes.
CI failures are unrelated
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50215:
URL: https://github.com/apache/arrow/pull/50215
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3440903266
##########
cpp/src/arrow/filesystem/s3fs_module_test.cc:
##########
@@ -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()) {
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50215:
URL: https://github.com/apache/arrow/pull/50215#issuecomment-4766346516
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40410: [C++] Skip only s3fs-tests and s3fs-module-tests that require MinIO if MinIO is not available [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50215:
URL: https://github.com/apache/arrow/pull/50215#discussion_r3435665445
##########
cpp/src/arrow/filesystem/s3_test_util.cc:
##########
@@ -192,6 +192,15 @@ void MinioTestEnvironment::SetUp() {
MakeReadaheadGenerator(std::move(impl_->server_generator_), pool->GetCapacity());
}
+bool MinioTestEnvironment::IsAvailable() {
+ if (!checked_) {
+ ::arrow::util::Process process;
+ available_ = process.SetExecutable(kMinioExecutableName).ok();
+ checked_ = true;
+ }
+ return available_;
+}
Review Comment:
`MinioTestServer::Start()` supports running against an external S3-compatible service when `ARROW_TEST_S3_CONNECT_STRING`/`ARROW_TEST_S3_ACCESS_KEY`/`ARROW_TEST_S3_SECRET_KEY` are set, which does not require the `minio` executable. With the current implementation, `IsAvailable()` returns false when `minio` isn't on `PATH`, causing those externally-configured test runs to be incorrectly skipped.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org