michael-s-molina commented on code in PR #26909:
URL: https://github.com/apache/superset/pull/26909#discussion_r1475127136
##########
tests/integration_tests/explore/permalink/api_tests.py:
##########
@@ -39,7 +39,7 @@
@pytest.fixture
def chart(app_context, load_world_bank_dashboard_with_slices) -> Slice:
session: Session = app_context.app.appbuilder.get_session
Review Comment:
```suggestion
```
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1748,7 +1748,7 @@ def test_export_dataset(self):
assert rv.status_code == 200
cli_export = export_to_dict(
- session=db.session,
+ db.session.db.session,
Review Comment:
```suggestion
```
##########
tests/unit_tests/charts/dao/dao_tests.py:
##########
@@ -57,20 +58,18 @@ def
test_slice_find_by_id_skip_base_filter(session_with_data: Session) -> None:
def test_datasource_find_by_id_skip_base_filter_not_found(
- session_with_data: Session,
+ session: Session,
) -> None:
from superset.daos.chart import ChartDAO
- result = ChartDAO.find_by_id(
- 125326326, session=session_with_data, skip_base_filter=True
- )
+ result = ChartDAO.find_by_id(125326326, skip_base_filter=True)
assert result is None
-def test_add_favorite(session_with_data: Session) -> None:
+def test_add_favorite(session: Session) -> None:
Review Comment:
```suggestion
def test_add_favorite() -> None:
```
##########
tests/unit_tests/charts/dao/dao_tests.py:
##########
@@ -37,18 +38,18 @@ def session_with_data(session: Session) ->
Iterator[Session]:
datasource_name="tmp_perm_table",
slice_name="slice_name",
)
- session.add(slice_obj)
+ db.session.add(slice_obj)
- session.commit()
+ db.session.commit()
yield session
- session.rollback()
+ db.session.rollback()
def test_slice_find_by_id_skip_base_filter(session_with_data: Session) -> None:
Review Comment:
```suggestion
def test_slice_find_by_id_skip_base_filter() -> None:
```
##########
tests/unit_tests/charts/dao/dao_tests.py:
##########
@@ -82,10 +81,10 @@ def test_add_favorite(session_with_data: Session) -> None:
assert len(ChartDAO.favorited_ids([chart])) == 1
-def test_remove_favorite(session_with_data: Session) -> None:
+def test_remove_favorite(session: Session) -> None:
Review Comment:
```suggestion
def test_remove_favorite() -> None:
```
##########
tests/unit_tests/commands/importers/v1/assets_test.py:
##########
@@ -35,14 +35,14 @@ def test_import_new_assets(mocker: MockFixture, session:
Session) -> None:
"""
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/dao/tag_test.py:
##########
@@ -70,7 +70,7 @@ def test_remove_user_favorite_tag(mocker):
# Check that users_favorited no longer contains the user
assert mock_user not in mock_tag.users_favorited
- # Check that the session was committed
+ # Check that the db.session.was committed
Review Comment:
```suggestion
# Check that the db.session was committed
```
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -25,17 +25,18 @@
def test_query_dao_save_metadata(session: Session) -> None:
Review Comment:
```suggestion
def test_query_dao_save_metadata() -> None:
```
##########
tests/unit_tests/dashboards/dao_tests.py:
##########
@@ -23,9 +23,10 @@
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
+ from superset import db
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/databases/ssh_tunnel/commands/update_test.py:
##########
@@ -25,28 +25,31 @@
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
+ from superset import db
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/charts/dao/dao_tests.py:
##########
@@ -20,14 +20,15 @@
import pytest
from sqlalchemy.orm.session import Session
+from superset import db
from superset.utils.core import DatasourceType
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
from superset.models.slice import Slice
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
This change looks weird because the function is receiving a session via
parameter to enhance it with data but in reality the parameter is not being
used to process the request. Maybe we should remove the `session` parameter and
rename the function to `add_data_to_session` or `add_slice_to_session`.
##########
tests/unit_tests/charts/commands/importers/v1/import_test.py:
##########
@@ -38,14 +39,14 @@ def test_import_chart(mocker: MockFixture, session:
Session) -> None:
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Should we remove `test_import_chart`'s `session` parameter as well?
##########
tests/unit_tests/charts/dao/dao_tests.py:
##########
@@ -57,20 +58,18 @@ def
test_slice_find_by_id_skip_base_filter(session_with_data: Session) -> None:
def test_datasource_find_by_id_skip_base_filter_not_found(
- session_with_data: Session,
+ session: Session,
Review Comment:
```suggestion
```
##########
tests/unit_tests/tags/commands/update_test.py:
##########
@@ -30,7 +31,7 @@ def session_with_data(session: Session):
from superset.models.sql_lab import Query, SavedQuery
from superset.tags.models import Tag
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -38,21 +39,21 @@
def database1(session: Session) -> Iterator["Database"]:
Review Comment:
```suggestion
def database1() -> Iterator["Database"]:
```
##########
tests/unit_tests/conftest.py:
##########
@@ -41,15 +41,15 @@
@pytest.fixture
def get_session(mocker: MockFixture) -> Callable[[], Session]:
"""
- Create an in-memory SQLite session to test models.
+ Create an in-memory SQLite db.session.to test models.
Review Comment:
```suggestion
Create an in-memory SQLite db.session to test models.
```
##########
tests/unit_tests/dao/dataset_test.py:
##########
@@ -46,8 +47,8 @@ def test_validate_update_uniqueness(session: Session) -> None:
schema="dev",
database=database,
)
- session.add_all([database, dataset1, dataset2])
- session.flush()
+ db.session.add_all([database, dataset1, dataset2])
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/databases/dao/dao_tests.py:
##########
@@ -23,31 +23,32 @@
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/config_test.py:
##########
@@ -81,7 +83,7 @@ def test_table(session: Session) -> "SqlaTable":
from superset.connectors.sqla.models import SqlaTable, TableColumn
from superset.models.core import Database
- engine = session.get_bind()
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/columns/test_models.py:
##########
@@ -24,9 +24,10 @@ def test_column_model(session: Session) -> None:
"""
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/charts/test_post_processing.py:
##########
@@ -1965,12 +1965,13 @@ def test_apply_post_process_json_format_data_is_none():
def test_apply_post_process_verbose_map(session: Session):
Review Comment:
```suggestion
def test_apply_post_process_verbose_map():
```
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -28,6 +28,8 @@
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import db
+
def test_filter_by_uuid(
session: Session,
Review Comment:
```suggestion
```
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -183,31 +187,32 @@ def test_query_dao_stop_query_not_running(
status=QueryStatus.FAILED,
)
- session.add(db)
- session.add(query_obj)
+ db.session.add(database)
+ db.session.add(query_obj)
from superset.daos.query import QueryDAO
QueryDAO.stop_query(query_obj.client_id)
- query = session.query(Query).one()
+ query = db.session.query(Query).one()
assert query.status == QueryStatus.FAILED
def test_query_dao_stop_query_failed(
mocker: MockFixture, app: Any, session: Session
Review Comment:
```suggestion
mocker: MockFixture, app: Any,
```
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -231,23 +236,24 @@ def test_query_dao_stop_query_failed(
with pytest.raises(SupersetCancelQueryException):
QueryDAO.stop_query(query_obj.client_id)
- query = session.query(Query).one()
+ query = db.session.query(Query).one()
assert query.status == QueryStatus.RUNNING
def test_query_dao_stop_query(mocker: MockFixture, app: Any, session: Session)
-> None:
Review Comment:
```suggestion
def test_query_dao_stop_query(mocker: MockFixture, app: Any) -> None:
```
##########
tests/unit_tests/dashboards/dao_tests.py:
##########
@@ -36,18 +37,16 @@ def session_with_data(session: Session) ->
Iterator[Session]:
published=True,
)
- session.add(dashboard_obj)
- session.commit()
+ db.session.add(dashboard_obj)
+ db.session.commit()
yield session
- session.rollback()
+ db.session.rollback()
-def test_add_favorite(session_with_data: Session) -> None:
+def test_add_favorite(session: Session) -> None:
Review Comment:
```suggestion
def test_add_favorite() -> None:
```
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -151,25 +154,26 @@ def test_query_dao_stop_query_not_found(
with pytest.raises(QueryNotFoundException):
QueryDAO.stop_query("foo2")
- query = session.query(Query).one()
+ query = db.session.query(Query).one()
assert query.status == QueryStatus.RUNNING
def test_query_dao_stop_query_not_running(
mocker: MockFixture, app: Any, session: Session
Review Comment:
```suggestion
mocker: MockFixture, app: Any,
##########
tests/unit_tests/dashboards/dao_tests.py:
##########
@@ -59,12 +58,10 @@ def test_add_favorite(session_with_data: Session) -> None:
assert len(DashboardDAO.favorited_ids([dashboard])) == 1
-def test_remove_favorite(session_with_data: Session) -> None:
+def test_remove_favorite(session: Session) -> None:
Review Comment:
```suggestion
def test_remove_favorite() -> None:
```
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -116,18 +118,19 @@ def test_query_dao_get_queries_changed_after(session:
Session) -> None:
def test_query_dao_stop_query_not_found(
mocker: MockFixture, app: Any, session: Session
Review Comment:
```suggestion
mocker: MockFixture, app: Any,
```
##########
tests/unit_tests/databases/commands/importers/v1/import_test.py:
##########
@@ -37,11 +38,11 @@ def test_import_database(mocker: MockFixture, session:
Session) -> None:
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/dao/queries_test.py:
##########
@@ -48,30 +49,31 @@ def test_query_dao_save_metadata(session: Session) -> None:
results_key="abc",
)
- session.add(db)
- session.add(query_obj)
+ db.session.add(database)
+ db.session.add(query_obj)
from superset.daos.query import QueryDAO
- query = session.query(Query).one()
+ query = db.session.query(Query).one()
QueryDAO.save_metadata(query=query, payload={"columns": []})
assert query.extra.get("columns", None) == []
def test_query_dao_get_queries_changed_after(session: Session) -> None:
Review Comment:
```suggestion
def test_query_dao_get_queries_changed_after() -> None:
```
##########
tests/unit_tests/databases/ssh_tunnel/commands/delete_test.py:
##########
@@ -24,31 +24,32 @@
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/dashboards/commands/importers/v1/import_test.py:
##########
@@ -38,12 +39,12 @@ def test_import_dashboard(mocker: MockFixture, session:
Session) -> None:
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -79,13 +80,13 @@ def database2(session: Session) -> Iterator["Database"]:
sqlalchemy_uri="sqlite:///database2.db",
allow_dml=False,
)
- session.add(database)
- session.commit()
+ db.session.add(database)
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/datasource/dao_tests.py:
##########
@@ -32,10 +33,10 @@ def session_with_data(session: Session) ->
Iterator[Session]:
from superset.models.sql_lab import Query, SavedQuery
from superset.tables.models import Table
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/datasets/api_tests.py:
##########
@@ -19,6 +19,8 @@
from sqlalchemy.orm.session import Session
+from superset import db
+
def test_put_invalid_dataset(
session: Session,
Review Comment:
```suggestion
```
##########
tests/unit_tests/datasets/commands/export_test.py:
##########
@@ -20,6 +20,8 @@
from sqlalchemy.orm.session import Session
+from superset import db
+
def test_export(session: Session) -> None:
Review Comment:
```suggestion
def test_export() -> None:
```
##########
tests/unit_tests/tables/test_models.py:
##########
@@ -14,11 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
# pylint: disable=import-outside-toplevel, unused-argument
-
from sqlalchemy.orm.session import Session
+from superset import db
+
def test_table_model(session: Session) -> None:
Review Comment:
```suggestion
def test_table_model() -> None:
```
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -46,12 +47,12 @@ def test_import_dataset(mocker: MockFixture, session:
Session) -> None:
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/datasets/dao/dao_tests.py:
##########
@@ -20,28 +20,30 @@
import pytest
from sqlalchemy.orm.session import Session
+from superset import db
+
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
from superset.connectors.sqla.models import SqlaTable
from superset.models.core import Database
- engine = session.get_bind()
+ engine = db.session.get_bind()
Review Comment:
Same comment as before about enhancing the session with data.
##########
tests/unit_tests/tags/commands/create_test.py:
##########
@@ -56,12 +60,12 @@ def session_with_data(session: Session):
published=True,
)
- session.add(slice_obj)
- session.add(db)
- session.add(saved_query)
- session.add(dashboard_obj)
- session.commit()
- yield session
+ db.session.add(slice_obj)
+ db.session.add(database)
+ db.session.add(saved_query)
+ db.session.add(dashboard_obj)
+ db.session.commit()
+ yield db.session
def test_create_command_success(session_with_data: Session, mocker:
MockFixture):
Review Comment:
```suggestion
def test_create_command_success(mocker: MockFixture):
```
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -125,7 +125,7 @@ def test_sql_lab_insert_rls_as_subquery(
from superset.sql_lab import execute_sql_statement
from superset.utils.core import RowLevelSecurityFilterType
- engine = session.connection().engine
+ engine = db.session.connection().engine
Review Comment:
Remove `session` parameter?
##########
tests/unit_tests/tags/commands/update_test.py:
##########
@@ -117,8 +120,8 @@ def test_update_command_success_duplicates(
from superset.models.slice import Slice
from superset.tags.models import ObjectType, TaggedObject
- dashboard = session_with_data.query(Dashboard).first()
- chart = session_with_data.query(Slice).first()
+ dashboard = db.session.query(Dashboard).first()
Review Comment:
Remove `session_with_data` parameter?
##########
tests/unit_tests/tags/commands/create_test.py:
##########
@@ -18,18 +18,20 @@
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import db
from superset.utils.core import DatasourceType
@pytest.fixture
def session_with_data(session: Session):
+ from superset import db
Review Comment:
Same comment as before about enhancing the session with data.
--
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]
