Get view names along with tables #498
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
==========================================
+ Coverage 87.96% 88.00% +0.04%
==========================================
Files 24 24
Lines 1030 1034 +4
==========================================
+ Hits 906 910 +4
Misses 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -69,9 +69,12 @@ def check_draft_datasource_config( | |||
| created_resources=[], | |||
| ) | |||
|
|
|||
| def _get_table_names(self, datasource: Datasource) -> list[str]: | |||
| def _get_table_names(self, datasource: Datasource, *, include_views: bool) -> list[str]: | |||
There was a problem hiding this comment.
Rename to _get_table_and_view_names and remove the boolean flag. We don't have a need for the conditional and it makes the API less clear.
| @@ -1,6 +1,6 @@ | |||
| [tool.poetry] | |||
| name = "great_expectations_cloud" | |||
| version = "20241017.0.dev1" | |||
| version = "20241021.0.dev0" | |||
| _get_table_names_spy.assert_called_with( | ||
| datasource=datasource_cls(**datasource_config), include_views=True | ||
| ) | ||
| _update_table_names_list_spy.assert_called_with(config_id=config_id, table_names=table_names) |
There was a problem hiding this comment.
These assertions don't add any value to the test and make refactoring harder. Testing that a method calls a private helper makes no guarantee that what the helper is doing is correct.
These tests is already verifying that the correct endpoint is called with the correct tables/views and that is enough
There was a problem hiding this comment.
Some tests here should guarantee that we return views as well
| inspector = mocker.Mock(spec=Inspector) | ||
| inspector.get_table_names.return_value = table_names | ||
| inspector.get_view_names.return_value = view_names |
There was a problem hiding this comment.
I would expect this test to fail since we're not getting different results. The test should guarantee that we're sending back the correct results
Return views along with table names.
Given that we no longer support
QueryAssettypes in GX-Cloud, users need to be able to import views along with tables when adding assets from the UI.https://docs.sqlalchemy.org/en/20/core/reflection.html#sqlalchemy.engine.reflection.Inspector