-
Notifications
You must be signed in to change notification settings - Fork 412
Run Catalog integration tests against REST Catalog impls #2482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I think under the contributing or dev or somewhere in the docs can we specify how a user would do this? |
3eafcf4 to
2e20187
Compare
|
@jayceslesar I added docs in contributing that explains how to use this. |
|
This is great! thanks for working on this! Im thinking for future next steps would be on thinking how we can allow for custom catalogs to ignore tests that are not supported by that catalog like for example catalogs that do not support views or multi-level namespaces, and so on. We should somehow allow the users to selectively skip tests, like for example java CatalogTests |
2e20187 to
933261c
Compare
kevinjqliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really interesting idea. Thanks for adding it!
933261c to
85dc168
Compare
|
@kevinjqliu @Fokko @gabeiglio rebased and comments addressed! Thanks a lot |
|
@rambleraptor Why do I still need to approve your runs? :D |
Fokko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @rambleraptor 🙌
|
@Fokko if there's a way for you to let me do CI runs, that would be awesome! |
|
@rambleraptor I was under the impression that it should run after a merge. Let me check with infra |
|
I've seen in past non-Apache OSS projects that automatic CI runs are limited to a specific set of people (typically committers, not always). The idea behind that is that you don't want random malicious people sending PRs with Bitcoin mining scripts in them |
|
@Fokko regardless, I changed the fixture name which should enable the tests to work. I'll try getting my integration test setup in a better situation to avoid having to rely on CI so much |
|
Thanks @rambleraptor for the PR and @gabeiglio @Fokko for the review! |
Closes #2481
Rationale for this change
This allows users to run our test suite against their REST Catalog implementations.
Are these changes tested?
Test-only change
Are there any user-facing changes?