Conversation
tests/catalog/test_hive.py
Outdated
|
|
||
| @pytest.mark.parametrize("hive2_compatible", [True, False]) | ||
| @patch("time.time", MagicMock(return_value=12345)) | ||
| def test_register_table( |
There was a problem hiding this comment.
How would you feel about moving this test to tests/integration/test_register_table.py? We can do a test both for Hive and the Rest catalog. The big upside here is that we don't have to mock everything.
There was a problem hiding this comment.
Integration test against real local Hivemetastore is better, I'll try to implement this. Did not notice the great integration test coverage before.
| @pytest.mark.integration | ||
| def test_hive_register_table( | ||
| session_catalog: HiveCatalog, | ||
| ) -> None: |
There was a problem hiding this comment.
Thanks @JoniKet This is looking great, thanks for changing these tests 👍
The session_catalog is a RestCatalog, instead you want to use:
| @pytest.mark.integration | |
| def test_hive_register_table( | |
| session_catalog: HiveCatalog, | |
| ) -> None: | |
| @pytest.mark.integration | |
| def test_hive_register_table( | |
| session_catalog_hive: HiveCatalog, | |
| ) -> None: |
It would be even better to test both:
| @pytest.mark.integration | |
| def test_hive_register_table( | |
| session_catalog: HiveCatalog, | |
| ) -> None: | |
| @pytest.mark.integration | |
| @pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive"), pytest.lazy_fixture("session_catalog")]) | |
| def test_hive_register_table(catalog: Catalog) -> None: |
This will run the test twice, once for the RestCatalog, and once for the HiveCatalog. This way we don't have to duplicate the tests for each of the catalog.
There was a problem hiding this comment.
Added a commit where the catalog is parametrised for the unit test. Cool, did not know it is possible to parametrise fixture that way with pytest
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Perfect, thanks @JoniKet for adding this, and thanks @kevinjqliu for the reviews! 🚀
3 participants
Added the register_table operation for Hive metastore. Had use case where there were existing tables in AWS S3, which needed to be registered to Hive metastore. Registering table with PyIceberg was not possible yet in Hive metastore.
Testing
Tested with the attached unit tests and also against remote hive metastore leveraging PyIceberg.