Skip to content

Conversation

@tmater
Copy link
Contributor

@tmater tmater commented Nov 19, 2025

Summary

Disables 5 view catalog tests for cloud storage providers (S3, ADLS, GCS) that use @TempDir. The ViewCatalogTests base class uses Paths.get() with the temp directory, which doesn't support cloud storage paths.

Follow-up to #2871.

Background

These tests inherit from Iceberg's ViewCatalogTests, which passes a @TempDir path to Paths.get(). Since Paths.get() only accepts local filesystem paths, it cannot handle cloud storage URIs. For example, when @TempDir provides /tmp/junit123, Paths.get() converts it to a URI like file:///tmp/junit123. But for cloud storage base paths like s3://bucket/path, it would incorrectly create s3:/bucket/path (single slash) instead of the valid s3://bucket/path format.

These tests are disabled to prevent false positives - they would pass by testing against local filesystem instead of actually validating cloud storage behavior.

Changes

Base test classes - Added @Disabled overrides for 5 tests:

  • PolarisRestCatalogViewAdlsIntegrationTestBase.java
  • PolarisRestCatalogViewGcsIntegrationTestBase.java
  • PolarisRestCatalogViewS3IntegrationTestBase.java

Concrete test classes - Removed reflection workaround:

  • RestCatalogViewAdlsIT.java
  • RestCatalogViewGcsIT.java
  • RestCatalogViewS3IT.java

Testing

Run cloud tests: ./gradlew cloudTest

Future Work

Will follow up with changes to Iceberg's ViewCatalogTests to avoid using Paths.get() with @TempDir. Once that's done, these tests can be re-enabled.

Disable tests from ViewCatalogTests for cloud storage integration tests
(S3, ADLS, GCS) that would otherwise use @tempdir. Since @tempdir internally
uses Paths.get, it cannot point to cloud storage paths. These tests remain
enabled for file-based integration tests.
@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Nov 19, 2025
@tmater tmater marked this pull request as ready for review November 19, 2025 19:02
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Good catch, @tmater !

I think disabling these tests for now is a reasonable approach.

However, in the log run, should we rather fix the tests to allow any (configurable) storage URIs?

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Nov 19, 2025
@tmater
Copy link
Contributor Author

tmater commented Nov 19, 2025

Thank you for the quick review, @dimas-b and @snazy!

@dimas-b, I’ve already started experimenting with some changes. I don’t expect them to be too disruptive, but we’ll also need to bump our Iceberg version for that. I figured it’s better to disable these for now.

@dimas-b
Copy link
Contributor

dimas-b commented Nov 19, 2025

Yes, I'm sure an Iceberg change will be needed to make the test reusable in cloud environments.

@dimas-b dimas-b merged commit 89eb4cc into apache:main Nov 21, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants