Rest catalog integration testing #1469
Conversation
…y migration from the mock tests
…/AhmedNader42/iceberg-python into rest-catalog-integration-testing
|
I had a look at the failing tests, and it appears the issue is in the sigv4 authentication option. But I can't reproduce it on local. Any ideas where to proceed from here? |
|
Hello @kevinjqliu, Please can we have a review for this PR |
| def test_namespace_exists(catalog: RestCatalog) -> None: | ||
| if not catalog.namespace_exists(TEST_NAMESPACE_IDENTIFIER): | ||
| @pytest.fixture(scope="function") | ||
| @pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog"), pytest.lazy_fixture("test_clean_up")]) |
There was a problem hiding this comment.
nit: you can use something like this to automatically run clean up after every test
https://stackoverflow.com/questions/22627659/run-code-before-and-after-each-test-in-py-test
@pytest.fixture(autouse=True)
def cleanup():
# Code that will run before your test, for example:
yield
# Code that will run after your test, for example:
| def test_clean_up(catalog: RestCatalog) -> None: | ||
| for namespaces_tuple in catalog.list_namespaces(): | ||
| namespace_name = namespaces_tuple[0] | ||
| if TEST_NAMESPACE_IDENTIFIER[0] in namespace_name: | ||
| for identifier in catalog.list_tables(namespace_name): | ||
| catalog.purge_table(identifier) | ||
| if catalog.namespace_exists(TEST_NAMESPACE_IDENTIFIER): | ||
| catalog.drop_namespace(TEST_NAMESPACE_IDENTIFIER) |
There was a problem hiding this comment.
since we only have 2 tables and 1 namespace:
TEST_NAMESPACE_IDENTIFIER = ("rest_integration_ns",)
TEST_TABLE_IDENTIFIER = ("rest_integration_ns", "rest_integration_tbl")
TEST_TABLE_IDENTIFIER_RENAME = ("rest_integration_ns", "renamed_rest_integration_tbl")
we can drop all tables in the TEST_NAMESPACE_IDENTIFIER namespace, and then remove the remove
|
|
||
| @pytest.mark.integration | ||
| @pytest.mark.parametrize("catalog,clean_up", [(pytest.lazy_fixture("session_catalog"), pytest.lazy_fixture("test_clean_up"))]) | ||
| def test_create_namespace_200(catalog: RestCatalog, clean_up: Any) -> None: |
There was a problem hiding this comment.
nit: the naming is confusing since we dont assert the 200 status code.
how about just test_create_namespace
| def test_create_namespace_if_exists_409(catalog: RestCatalog, clean_up: Any) -> None: | ||
| catalog.create_namespace(TEST_NAMESPACE_IDENTIFIER) | ||
| catalog.create_namespace_if_not_exists(TEST_NAMESPACE_IDENTIFIER) | ||
| assert TEST_NAMESPACE_IDENTIFIER in catalog.list_namespaces() |
There was a problem hiding this comment.
nit: similar to above with naming, the test name can be more descriptive. i think here we're testing that create_namespace_if_not_exists does not throw NamespaceAlreadyExistsError when the namespace already exists
| @pytest.mark.parametrize("catalog,clean_up", [(pytest.lazy_fixture("session_catalog"), pytest.lazy_fixture("test_clean_up"))]) | ||
| def test_list_namespaces_200(catalog: RestCatalog, clean_up: Any) -> None: | ||
| catalog.create_namespace(TEST_NAMESPACE_IDENTIFIER) | ||
| assert catalog.list_namespaces() == [("default",), TEST_NAMESPACE_IDENTIFIER] |
There was a problem hiding this comment.
nit: TEST_NAMESPACE_IDENTIFIER in catalog.list_namespaces() so we dont care about the other namespace
This PR resolves #1439 by adding integration tests for the REST Catalog.
Functionality testing against the server can be simulated to a certain degree, but some checks are very hard to emulate for example, triggering a 500 response from the server or getting information from the request which is abstracted away by the catalog.
Here is the lists of the tests that were successfully created in this integration testing:
There are some tests that are either skipped or were not applicable to the integration test or producing incorrect results.
Here is the full list: