john-bodley commented on a change in pull request #15909:
URL: https://github.com/apache/superset/pull/15909#discussion_r677851659
##########
File path: tests/integration_tests/datasource_tests.py
##########
@@ -168,25 +158,21 @@ def test_save(self):
else:
self.assertEqual(resp[k], datasource_post[k])
- def save_datasource_from_dict(self, datasource_dict):
+ def save_datasource_from_dict(self, datasource_post):
data = dict(data=json.dumps(datasource_post))
resp = self.get_json_resp("/datasource/save/", data)
return resp
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
Review comment:
I have no idea how this test worked without first loading the example
data.
##########
File path: tests/integration_tests/datasource_tests.py
##########
@@ -26,30 +26,24 @@
from superset.datasets.commands.exceptions import DatasetNotFoundError
from superset.exceptions import SupersetException,
SupersetGenericDBErrorException
from superset.utils.core import get_example_database
+from tests.integration_tests.base_tests import db_insert_temp_object,
SupersetTestCase
from tests.integration_tests.fixtures.birth_names_dashboard import (
load_birth_names_dashboard_with_slices,
)
-
-from .base_tests import db_insert_temp_object, SupersetTestCase
-from .fixtures.datasource import datasource_post
+from tests.integration_tests.fixtures.datasource import get_datasource_post
class TestDatasource(SupersetTestCase):
def setUp(self):
- self.original_attrs = {}
Review comment:
I really have no idea why this logic is here and the `before_update`
method exposed its fragility. Rather than using instance variables to save
state and restore state, using a sub-transaction and rolling back seems
considerably cleaner.
##########
File path: tests/integration_tests/datasource_tests.py
##########
@@ -168,25 +158,21 @@ def test_save(self):
else:
self.assertEqual(resp[k], datasource_post[k])
- def save_datasource_from_dict(self, datasource_dict):
Review comment:
Hmmm. The local variable was never used but instead the global variable
was. Seems questionable.
##########
File path: tests/integration_tests/datasource_tests.py
##########
@@ -225,18 +207,15 @@ def test_save_duplicate_key(self):
},
]
)
- data = dict(data=json.dumps(datasource_post_copy))
+ data = dict(data=json.dumps(datasource_post))
resp = self.get_json_resp("/datasource/save/", data,
raise_on_error=False)
self.assertIn("Duplicate column name(s): <new column>", resp["error"])
def test_get_datasource(self):
self.login(username="admin")
- tbl = self.get_table_by_name("birth_names")
- self.datasource = ConnectorRegistry.get_datasource("table", tbl.id,
db.session)
-
- for key in self.datasource.export_fields:
- self.original_attrs[key] = getattr(self.datasource, key)
+ tbl = self.get_table(name="birth_names")
+ datasource_post = get_datasource_post()
datasource_post["id"] = tbl.id
Review comment:
Yikes! This was updating the global variable.
##########
File path: tests/integration_tests/base_tests.py
##########
@@ -254,13 +245,31 @@ def get_slice(
return slc
@staticmethod
- def get_table_by_name(name: str) -> SqlaTable:
- return get_table_by_name(name)
+ def get_table(
Review comment:
Cleanup to ensure consistency.
##########
File path: tests/integration_tests/tasks/async_queries_tests.py
##########
@@ -42,11 +42,6 @@
from tests.integration_tests.test_app import app
-def get_table_by_name(name: str) -> SqlaTable:
Review comment:
This doesn't need to be redefined. Adhering to the DRY principle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
