From 1e2fb090eff8640eed2408d60fb40d137c88b003 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 25 Sep 2024 15:56:27 -0400 Subject: [PATCH 1/2] feat: turn off autoflush on integration tests --- tests/integration_tests/conftest.py | 25 ++++-- .../utils/decorators_test.py | 84 +++++++++++++++++++ 2 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 tests/integration_tests/utils/decorators_test.py diff --git a/tests/integration_tests/conftest.py b/tests/integration_tests/conftest.py index 4f6ce10b0f9a..8a1748d5e231 100644 --- a/tests/integration_tests/conftest.py +++ b/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(), ) diff --git a/tests/integration_tests/utils/decorators_test.py b/tests/integration_tests/utils/decorators_test.py new file mode 100644 index 000000000000..2f01840a675d --- /dev/null +++ b/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() From 5309aaa00dade44097987baee19744f0231b33c7 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Thu, 26 Sep 2024 15:46:45 -0400 Subject: [PATCH 2/2] WIP --- tests/integration_tests/dashboard_tests.py | 1 + tests/integration_tests/fixtures/birth_names_dashboard.py | 1 + tests/integration_tests/fixtures/world_bank_dashboard.py | 1 + tests/integration_tests/model_tests.py | 1 + 4 files changed, 4 insertions(+) diff --git a/tests/integration_tests/dashboard_tests.py b/tests/integration_tests/dashboard_tests.py index 78218744b362..a7e58ea2f414 100644 --- a/tests/integration_tests/dashboard_tests.py +++ b/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 diff --git a/tests/integration_tests/fixtures/birth_names_dashboard.py b/tests/integration_tests/fixtures/birth_names_dashboard.py index 084cbcb2a6d3..10890ef8bd23 100644 --- a/tests/integration_tests/fixtures/birth_names_dashboard.py +++ b/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 diff --git a/tests/integration_tests/fixtures/world_bank_dashboard.py b/tests/integration_tests/fixtures/world_bank_dashboard.py index 983c0fba27b4..37d7b579f98a 100644 --- a/tests/integration_tests/fixtures/world_bank_dashboard.py +++ b/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(): diff --git a/tests/integration_tests/model_tests.py b/tests/integration_tests/model_tests.py index 08ae8e16d5f5..82cbe7f8dcc1 100644 --- a/tests/integration_tests/model_tests.py +++ b/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