fix: use InMemoryCatalog to avoid config collision in tests#3006
fix: use InMemoryCatalog to avoid config collision in tests#3006geruh wants to merge 1 commit intoapache:mainfrom
Conversation
| identifier = "default.test_nanosecond_support_on_catalog" | ||
|
|
||
| catalog = load_catalog("default", type="in-memory") | ||
| catalog = InMemoryCatalog("test-catalog") |
There was a problem hiding this comment.
looks good just want to make sure this doesn't break nightly builds or something else. @kevinjqliu might be able to trigger one
|
Really clever find! Do we want to consider having some kind of |
|
thanks for catching this! ideally we shouldn't let external factors (such as the content of i remember we disables parsing the env and config file here iceberg-python/tests/cli/test_console.py Lines 40 to 42 in 7e66ccb perhaps we can do this as the default for all tests |
|
Yeah, local config shouldn't bleed into pytest runs. The better fix is in the test infra rather than patching individual tests. I added a session-scoped autouse fixture in the root conftest that replaces Did a sanity test-coverage run and everything looks goood!!! |
Rationale for this change
Fixes a test failure that hits anyone with PyIceberg already configured.
The test uses
load_catalog("default", type="in-memory"), which merges your dev environment config with the test parameters. If you have adefaultcatalog defined in~/.pyiceberg.yamlwith something like uri: https://best-rest-catalog.com, the merge produces{"uri": "https://best-rest-catalog.com", "type": "in-memory"}. SQLAlchemy sees that https:// URI and tries to load it as a database dialect, which fails wuth:Therefore, this PR avoids using the local pyicberg.yaml file entirely!
Are these changes tested?
Yes. I created a conflicting ~/.pyiceberg.yaml with a default catalog pointing to an HTTPS REST endpoint, confirmed the old code fails, and verified the fix bypasses the config merge and works.
Are there any user-facing changes?
No