-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
feat: Expanded Parameters for Mysql #14680
Conversation
tests/databases/api_tests.py
Outdated
@@ -1454,3 +1454,25 @@ def test_validate_parameters_invalid_host(self, is_hostname_valid): | |||
}, | |||
] | |||
} | |||
|
|||
def test_validate_parameters_with_mysql(self): |
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.
I don't understand this test. If we're passing the correct parameters why do we expect the status code to be 422?
You probably only need to update this test (which should fail otherwise with your PR): https://github.com/apache/superset/pull/14208/files#diff-b8428a369bdee6a99d7d4da28161a32d50477174192f9fade2040a335a4f2e42R1254
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 huh, I had it with rv.status 200 and it was passing. But you're right.
f9e3795
to
6ffa0a9
Compare
6ffa0a9
to
1106437
Compare
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
0794ea0
to
229150b
Compare
Codecov Report
@@ Coverage Diff @@
## master #14680 +/- ##
==========================================
+ Coverage 77.51% 77.55% +0.04%
==========================================
Files 958 958
Lines 48560 48563 +3
Branches 5703 5703
==========================================
+ Hits 37642 37664 +22
+ Misses 10718 10699 -19
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
@mock.patch("superset.databases.api.get_available_engine_specs") | ||
@mock.patch("superset.databases.api.app") | ||
def test_available_with_mysql(self, app, get_available_engine_specs): |
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.
You could've combined this and the test above in a single one:
get_available_engine_specs.return_value = [
PostgresEngineSpec,
MySQLEngineSpec,
HanaEngineSpec,
]
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.
yeah! Originally it was this, but I thought that as we add engines to this list that this test might get unwieldy and huge so it might be better to split up each engine spec.
* added mysql form * revisions * Update superset/db_engine_specs/mysql.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * added ssl and mysql testing Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* added mysql form * revisions * Update superset/db_engine_specs/mysql.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * added ssl and mysql testing Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* added mysql form * revisions * Update superset/db_engine_specs/mysql.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * added ssl and mysql testing Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
MySQL databases can now use the expanded parameters provided by the BasicParametersMixin to create new databases.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Added unit tests.
ADDITIONAL INFORMATION