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
Snowflake integration tests #336
Conversation
@edublancas Would you check again why |
doc building has been fixed in master, please rebase |
@tonykploomber Please don't add additional reviewers |
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.
Added some comments
I see the tests are failing. |
Yeah...I am fixing those |
@idomic Ready for review |
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.
looks good, my main concern is that the trial expires in 6 days and it'll fail the CI.
@edublancas
…esting-with-snowflake
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.
@tonykploomber
lgtm! tested on windows.
I saw it's already merged but we can patch a quick fix (added a comment)
|
||
|
||
def load_taxi_data(engine, table_name, index=True): | ||
table_name = table_name |
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.
We can remove this line
I don't see any docs for this in the developer guide: https://jupysql.ploomber.io/en/latest/community/developer-guide.html |
I assumed it's running as part of the integration tests: |
what happens if I want to run them locally? Looks like I'd need to set some environment variables |
Ok, I see your point. |
Good point. We should mention how to test this, system environment variables has to be set up in order to perform snowflake testing on local |
Describe your changes
The reason we need the dynamic table name is now we use one single snowflake connection endpoint to perform the test.
To prevent the race condition or any data consistency issue during the testing, each test session should have standalone environment: that's why we logically add the hashing to each test table.
Example table name:
taxi
->taxi-abc123
(6 chars uuid)Example case: Two people may push the commit to github at same time, those intergration testing might conflict to each other, since they are sharing the same database, same table name on snowflake
By the way, now the developer needs to add snowflake credential in the local environment variable:
(Please ask me $$$)
Issue number
Closes #247
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--336.org.readthedocs.build/en/336/