Skip to content

fix: use InMemoryCatalog to avoid config collision in tests#3006

Open
geruh wants to merge 1 commit intoapache:mainfrom
geruh:leak
Open

fix: use InMemoryCatalog to avoid config collision in tests#3006
geruh wants to merge 1 commit intoapache:mainfrom
geruh:leak

Conversation

@geruh
Copy link
Contributor

@geruh geruh commented Feb 6, 2026

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 a default catalog defined in ~/.pyiceberg.yaml with 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:

sqlalchemy.exc.NoSuchModuleError: Can't load plugin: sqlalchemy.dialects:https

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

@rambleraptor
Copy link
Contributor

rambleraptor commented Feb 6, 2026

Really clever find! Do we want to consider having some kind of load_catalog method that purposefully doesn't do this? This feels like something that could pop up again on an unrelated test.

@kevinjqliu
Copy link
Contributor

thanks for catching this! ideally we shouldn't let external factors (such as the content of ~/.pyiceberg.yaml) affect the test suite; that pollutes the tests imo.

i remember we disables parsing the env and config file here

# mock to prevent parsing ~/.pyiceberg.yaml or {PYICEBERG_HOME}/.pyiceberg.yaml
mocker.patch.dict(os.environ, values={"HOME": empty_home_dir_path, "PYICEBERG_HOME": empty_home_dir_path})
mocker.patch("pyiceberg.catalog._ENV_CONFIG", return_value=Config())

perhaps we can do this as the default for all tests

@geruh
Copy link
Contributor Author

geruh commented Feb 7, 2026

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 _ENV_CONFIG with a fresh Config() that skips file loading. Env vars still work for CI. This way load_catalog is safe to use in tests without worrying about config leaking in.

Did a sanity test-coverage run and everything looks goood!!!

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Smart fix, thanks @geruh for picking this up 👍

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.

5 participants