Add transaction tests to catalog integration tests#2371
Conversation
tests/integration/test_catalog.py
Outdated
| with table.transaction() as transaction: | ||
| with transaction.update_schema() as update_schema: | ||
| update_schema.add_column("new_col", IntegerType()) | ||
| expected_schema = update_schema._apply() |
There was a problem hiding this comment.
this apply is just to get the new schema back?
There was a problem hiding this comment.
I would rather hardcode the schema:
- This makes the test more readabile because you can see what you're asserting against.
- There are some idea's of avoid using "private" methods in the community ;)
jayceslesar
left a comment
jayceslesar
left a comment
There was a problem hiding this comment.
honestly wild how much we are covering with these tests
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This is awesome. Thank you!
|
Impressive work @gabeiglio. This would really help in the long run. |
|
@kevinjqliu @Fokko this might have fell of the radar, wondering if you folks could review it? |
sungwy
left a comment
sungwy
left a comment
There was a problem hiding this comment.
Thank you all for the reviews! @jayceslesar @rambleraptor @vinjai
And thanks @gabeiglio for the contribution. This is a great addition to the test suite.
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks for pinging me @gabeiglio This looks great 🙌
|
Thanks for working on this @gabeiglio, and thanks @vinjai, @rambleraptor, @jayceslesar and @sungwy for the review 🚀 |
6 participants
Just adding more tests to the catalog tests.