-
Notifications
You must be signed in to change notification settings - Fork 203
Fixes for failing tests #331
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
base: master
Are you sure you want to change the base?
Conversation
3c631b9 to
87157ec
Compare
| "Datasource definition not valid. Must be string or " | ||
| "dict or list of strings" | ||
| ) | ||
| if isinstance(datasource, list): |
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 revert of a change made in this commit, which I think was a breaking change to the generation of queries on union datasources. The PR doesn't offer additional details on this change, but the current code goes against the stated behaviour in the docstring and causes test failures.
It looks like the change in question never made it to a release, so I think it should be safe to just return to the previous behaviour.
| stream=True, | ||
| headers={"Content-Type": "application/json"}, | ||
| json={"query": "SELECT * FROM table", "context": {}, "header": False}, | ||
| json={"query": "SELECT * FROM table", "context": {}, "header": True}, |
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.
The header is set to True on line 352:
pydruid/tests/db/test_cursor.py
Line 352 in d480d07
| cursor = Cursor(url, header=True, ssl_client_cert="path/to/cert") |
So I have to assume that the False here is just a typo 🤷
| try: | ||
| dbapi_connection.execute(text("SELECT 1")) | ||
| except Exception as ex: | ||
| dbapi_connection.execute("SELECT 1") |
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 partial revert of this commit, which caused test failures because text("SELECT 1") != "SELECT 1" .
Based on this previous PR, using text() here causes issues for SQLAlchemy and should be avoided.
cc @betodealmeida - do you know why this was changed back to text()?
A few of these tests are failing on master currently:
I have made what I think is the most reasonable correction in each case, either reverting breaking code changes or fixing up tests and left comments on this PR explaining my rationale. Let me know what you think!