-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore(sql_lab): Added Unit Test for stop query exception #17464
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17464 +/- ##
==========================================
- Coverage 76.94% 76.74% -0.21%
==========================================
Files 1042 1042
Lines 56312 56312
Branches 7793 7793
==========================================
- Hits 43329 43216 -113
- Misses 12727 12840 +113
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
self.login(username="admin") | ||
|
||
with mock.patch("superset.views.core.db") as mock_superset_db: | ||
mock_superset_db.session.query().filter_by().one().return_value = query_mock |
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.
Oh, I didn't know you could use ()
instead of return_value
. Nice!
query_mock.sql = "SELECT *" | ||
query_mock.database = 1 | ||
query_mock.schema = "superset" | ||
query_mock.client_id = "foo" |
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.
Do we need to mock these attributes?
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.
No I deleted them.
query_mock.status = QueryStatus.RUNNING | ||
self.login(username="admin") | ||
|
||
with mock.patch("superset.views.core.db") as mock_superset_db: |
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.
Why not also patch the method with a decorator, like you did for cancel_query
?
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.
initially it was because it was giving me a 404 error and I saw this way of doing it repeated in various other integration tests. However, I played around with it, and if I @mock.patch(superset.views.core.db.session
then I can use it as a decorator in the same way that I use cancel_query.
So i changed it to that, because ti does make it a lot more consistent and readable.
"/superset/stop_query/", data={"form_data": json.dumps(form_data)}, | ||
) | ||
|
||
self.assertEqual(rv.status_code, 422) |
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.
Since me started using pylint naked asserts are preferred:
self.assertEqual(rv.status_code, 422) | |
assert rv.status_code == 422 |
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.
changed! I forgot about this.
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.
Awesome!
* added unit test * feedback implemented
SUMMARY
This PR adds a unit test for the changes made here: #17292
It checks to make sure that we are getting a 422 error when an engine does not support cancel_query in sql_lab.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION