feat: turn off autoflush on integration tests

tests/integration_tests/conftest.py

@@ -43,6 +43,15 @@
 ADMIN_SCHEMA_NAME = "admin_database"
 
 
+# When running tests it's important to disable autoflush to prevent dirty reads. If a
+# command modifies the database but doesn't explicitly commit the transaction, a
+# subsequent read won't see the data because we use READ COMMITTED isolation level. But
+# many integration tests write and read in the same session, and because by default
+# SQLAlchemy uses autoflush, the test would see data that in real world wouldn't be
+# there.
+db.session.configure(autoflush=False)
+
+
 @pytest.fixture
 def app_context():
     with app.app_context() as ctx:
@@ -74,7 +83,9 @@ def login_as_admin(login_as: Callable[..., None]):
 
 @pytest.fixture
 def create_user(app_context: AppContext):
-    def _create_user(username: str, role: str = "Admin", password: str = "general"):  # noqa: S107
+    def _create_user(
+        username: str, role: str = "Admin", password: str = "general"
+    ):  # noqa: S107
         security_manager.add_user(
             username,
             "firstname",
@@ -263,7 +274,8 @@ def virtual_dataset():
     dataset = SqlaTable(
         table_name="virtual_dataset",
         sql=(
-            dedent("""\
+            dedent(
+                """\
             SELECT 0 as col1, 'a' as col2, 1.0 as col3, NULL as col4, '2000-01-01 00:00:00' as col5, 1 as col6
             UNION ALL
             SELECT 1, 'b', 1.1, NULL, '2000-01-02 00:00:00', NULL
@@ -283,7 +295,8 @@ def virtual_dataset():
             SELECT 8 as col1, 'i' as col2, 1.8, NULL, '2000-01-09 00:00:00', 9
             UNION ALL
             SELECT 9 as col1, 'j' as col2, 1.9, NULL, '2000-01-10 00:00:00', 10
-        """)  # noqa: E501
+        """
+            )  # noqa: E501
         ),
         database=get_example_database(),
     )
@@ -312,7 +325,8 @@ def virtual_dataset_with_comments():
     dataset = SqlaTable(
         table_name="virtual_dataset_with_comments",
         sql=(
-            dedent("""\
+            dedent(
+                """\
             --COMMENT
             /*COMMENT*/
             WITH cte as (--COMMENT
@@ -323,7 +337,8 @@ def virtual_dataset_with_comments():
             UNION ALL/*COMMENT*/
             SELECT 1 as col1, 'f' as col2, 1.5, NULL, '2000-01-06 00:00:00', 6 --COMMENT
             UNION ALL--COMMENT
-            SELECT * FROM cte --COMMENT""")  # noqa: E501
+            SELECT * FROM cte --COMMENT"""
+            )  # noqa: E501
         ),
         database=get_example_database(),
     )

tests/integration_tests/dashboard_tests.py

@@ -103,6 +103,7 @@ def get_mock_positions(self, dash):
         return positions
 
     def test_get_dashboard(self):
+        db.session.flush()
         for dash in db.session.query(Dashboard):
             assert escape(dash.dashboard_title) in self.client.get(dash.url).get_data(
                 as_text=True

tests/integration_tests/fixtures/birth_names_dashboard.py

@@ -80,6 +80,7 @@ def _create_dashboards():
     dash = create_dashboard(slices)
     slices_ids_to_delete = [slice.id for slice in slices]
     dash_id_to_delete = dash.id
+    db.session.flush()
     return dash_id_to_delete, slices_ids_to_delete
 
 

tests/integration_tests/fixtures/world_bank_dashboard.py

@@ -63,6 +63,7 @@ def load_world_bank_data():
                 method="multi",
                 schema=get_example_default_schema(),
             )
+            db.session.flush()
 
     yield
     with app.app_context():

tests/integration_tests/model_tests.py

@@ -675,6 +675,7 @@ def test_data_for_slices_with_adhoc_column(self):
         )
         dashboard.slices.append(slc)
         datasource_info = slc.datasource.data_for_slices([slc])
+        metadata_db.session.flush()
         assert "database" in datasource_info
 
         # clean up and auto commit

tests/integration_tests/utils/decorators_test.py

@@ -0,0 +1,84 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from unittest.mock import patch
+
+import pytest
+
+from superset import db, security_manager
+from superset.commands.chart.fave import AddFavoriteChartCommand
+from superset.commands.chart.unfave import DelFavoriteChartCommand
+from superset.daos.chart import ChartDAO
+from superset.models.slice import Slice
+from superset.utils.core import override_user
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.energy_dashboard import (
+    load_energy_table_data,  # noqa: F401
+    load_energy_table_with_slice,  # noqa: F401
+)
+
+
+class TestTransactionDecorator(SupersetTestCase):
+    """
+    Test the transaction decorator.
+
+    These tests were created because we use the `transaction` decorator to perform
+    explicit commits in the code base. But because SQLAlchemy will autoflush by default,
+    some tests where doing dirty reads, and failing to capture that data was actually
+    not being written to the database.
+    """
+
+    @pytest.mark.usefixtures("load_energy_table_with_slice")
+    def test_with_commit(self) -> None:
+        """
+        Test that the decorator commits.
+        """
+        example_chart = db.session.query(Slice).all()[0]
+        assert example_chart is not None
+        ids = ChartDAO.favorited_ids([example_chart])
+        assert example_chart.id not in ids
+
+        with override_user(security_manager.find_user("admin")):
+            AddFavoriteChartCommand(example_chart.id).run()
+            ids = ChartDAO.favorited_ids([example_chart])
+            assert example_chart.id in ids
+
+            DelFavoriteChartCommand(example_chart.id).run()
+            ids = ChartDAO.favorited_ids([example_chart])
+            assert example_chart.id not in ids
+
+    @pytest.mark.usefixtures("load_energy_table_with_slice")
+    def test_without_commit(self) -> None:
+        """
+        Test that autoflush is off.
+
+        In this test we mock the `transaction` decorator so it doesn't autocommit. Since
+        the integration tests are configured with autoflush off, we should not see the
+        data in the same session.
+        """
+        with patch.object(db.session, "commit") as commit:
+            example_chart = db.session.query(Slice).all()[0]
+            assert example_chart is not None
+            ids = ChartDAO.favorited_ids([example_chart])
+            assert example_chart.id not in ids
+
+            with override_user(security_manager.find_user("admin")):
+                AddFavoriteChartCommand(example_chart.id).run()
+                ids = ChartDAO.favorited_ids([example_chart])
+                assert example_chart.id not in ids
+
+            commit.assert_called()