Add catalog properties for Catalog Tests#2982
Conversation
|
this does make tests a little nicer but also is it worth publicly exposing these methods just for making tests nicer? |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @rambleraptor for extending the tests, these have shown to be very valuable.
I'm not sure about all the methods, I think they might be misleading and I think they add a lot of noise in the public API.
|
I removed those methods from the public_api and made them test only methods that take in the catalog. These methods match up with what the Java implementation does. The REST Catalog ones are overridable via properties, but the defaults are the same defaults we currently assume in the tests. |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Nice one @rambleraptor Thanks for updating it, let's move this in 👍
|
i think adding not sure why this wasnt caught in CI 🤔 |
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
Context:
#2982 (comment)
Adding imports to `conftest.py` may break the nightly build pipeline.
(For example BigQuery in #2982).
This is because [nightly wheel build
tests](https://github.com/apache/iceberg-python/blob/95f6273b23524c6238aafb57fa06e693ef83d6ef/.github/workflows/pypi-build-artifacts.yml#L74-L75)
run in a narrower dependency set (`--only-group dev`), so new imports
could cause test collection to fail.
This PR inlines the imports in `conftest.py` and also include a smoke
test in CI to catch this problem going forward
## Are these changes tested?
yes, nightly build works again
https://github.com/apache/iceberg-python/actions/runs/22285169782
## Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label.
-->
4 participants
Rationale for this change
I've added a list of catalog-level properties to help the catalog tests. The goal is that the catalog tests work off of "features" and don't have exceptions for various catalogs.
As part of this, I discovered a discrepancy where HiveCatalog throws a different error than everyone else. I'm happy to change it back.
Are these changes tested?
Mostly a test change.
Are there any user-facing changes?