chore(tests): Remove unnecessary/problematic app contexts #28159

john-bodley · 2024-04-20T02:10:08Z

SUMMARY

As part of a large change related to [SIP-99B] Proposal for (re)defining a "unit of work", I'm in the process of creating some PRs to clean up some unnecessary/problematic logic to aid with future testing/debugging.

Specifically it seems like at times we're overly pushing an application context where, per the documentation, it states:

Only push a context exactly where and for how long it’s needed for each test. Do not push an application context globally for every test, as that can interfere with how the session is cleaned up.

This is likely the cause of a number of issues I was running into with expired objects in the session not being refreshed.

Finally this PR removes all db.session.expire_all() statements which are not needed. Per my previous statement, these likely were added due to some inconsistencies with how the session was being managed during testing.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue: [SIP-99B] Proposal for (re)defining a "unit of work" #25108
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2024-04-20T16:32:32Z

tests/integration_tests/fixtures/unicode_dashboard.py



 @pytest.fixture()
 def load_unicode_dashboard_with_slice(load_unicode_data):
    slice_name = "Unicode Cloud"
-    with app.app_context():


Unnecessary due to fixture inheritance.

john-bodley · 2024-04-20T16:32:36Z

tests/integration_tests/fixtures/unicode_dashboard.py

-        dash = _create_unicode_dashboard(slice_name, position)
-        yield
-        _cleanup(dash, slice_name)
+    dash = _create_unicode_dashboard(slice_name, position)


Unnecessary due to fixture inheritance.

john-bodley · 2024-04-20T16:36:45Z

tests/integration_tests/access_tests.py

@@ -81,36 +81,6 @@
 SCHEMA_ACCESS_ROLE = "schema_access_role"


-class TestRequestAccess(SupersetTestCase):


This class had no tests associated with it and thus is obsolete.

john-bodley · 2024-04-20T17:10:26Z

tests/integration_tests/dashboard_tests.py

@@ -58,39 +58,36 @@
 class TestDashboard(SupersetTestCase):
    @pytest.fixture
    def load_dashboard(self):
-        with app.app_context():


The app context is not needed within the confines of a derived Flask-Testing class.

john-bodley · 2024-04-20T17:10:44Z

tests/integration_tests/dashboards/security/security_dataset_tests.py

@@ -37,39 +37,36 @@
 class TestDashboardDatasetSecurity(DashboardTestCase):
    @pytest.fixture
    def load_dashboard(self):
-        with app.app_context():


The app context is not needed within the confines of a derived Flask-Testing class.

john-bodley · 2024-04-20T17:11:39Z

tests/integration_tests/fixtures/birth_names_dashboard.py

-    data_loader.load_table(birth_names_table)
-    yield
-    data_loader.remove_table(birth_names_table.table_name)
+    with app.app_context():


Due to inheritance of fixtures only the outer fixture requires instantiating the app context.

john-bodley · 2024-04-20T17:33:05Z

tests/integration_tests/databases/commands/csv_upload_test.py

@@ -96,16 +97,14 @@ def get_upload_db():
    return db.session.query(Database).filter_by(database_name=CSV_UPLOAD_DATABASE).one()


-@pytest.fixture(scope="function")


The function scope is the default scope.

john-bodley · 2024-04-20T17:48:58Z

tests/integration_tests/reports/alert_tests.py

    get_user,
 ) -> None:
    from superset.commands.report.alert import AlertCommand
    from superset.reports.models import ReportSchedule

-    with app.app_context():


The pushed app context is already being leveraged via the app_context fixture specified on line 65.

john-bodley · 2024-04-20T18:07:21Z

tests/integration_tests/reports/scheduler_tests.py

@@ -35,150 +35,144 @@ def owners(get_user) -> list[User]:
    return [get_user("admin")]


-@pytest.mark.usefixtures("owners")


No need to both specify the use of the fixture here in addition to passing it in to the function.

john-bodley · 2024-04-20T18:09:33Z

tests/integration_tests/utils_tests.py

@@ -685,20 +685,19 @@ def test_parse_js_uri_path_items_item_optional(self):
        self.assertIsNotNone(parse_js_uri_path_item("item"))

    def test_get_stacktrace(self):
-        with app.app_context():


The app context is not needed within the confines of a derived Flask-Testing class.

john-bodley · 2024-04-20T18:13:14Z

tests/integration_tests/utils/core_tests.py

@@ -82,5 +82,4 @@ def test_form_data_to_adhoc_incorrect_clause_type():
    form_data = {"where": "1 = 1", "having": "count(*) > 1"}

    with pytest.raises(ValueError):
-        with app.app_context():


No need for an app context to remap the form-data.

john-bodley · 2024-04-20T18:19:31Z

tests/integration_tests/security/analytics_db_safety_tests.py

@@ -84,10 +84,9 @@
 def test_check_sqlalchemy_uri(
    sqlalchemy_uri: str, error: bool, error_message: Optional[str]
 ):
-    with app.app_context():


Unnecessary.

john-bodley · 2024-04-20T20:51:38Z

tests/integration_tests/annotation_layers/fixtures.py

@@ -70,36 +68,35 @@ def _insert_annotation(


 @pytest.fixture()
-def create_annotation_layers():
+def create_annotation_layers(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T20:54:13Z

tests/integration_tests/charts/data/api_tests.py

-    with app.app_context():
-        if backend() == "hive":
-            pytest.skip("Skipping tests for Hive backend")
+def skip_by_backend(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T20:54:57Z

tests/integration_tests/conftest.py

-    # relying on `tests.integration_tests.test_app.app` leveraging an  `app` fixture which is purposely
-    # scoped to the function level to ensure tests remain idempotent.
+    # relying on `tests.integration_tests.test_app.app` leveraging an `app` fixture
+    # which is purposely scoped to the function level to ensure tests remain idempotent.


The existing app_context fixture cannot be used due to different scopes.

john-bodley · 2024-04-20T20:57:23Z

tests/integration_tests/databases/commands/csv_upload_test.py

-    with app.app_context():
-        yield from _setup_csv_upload()
+@pytest.fixture()
+def setup_csv_upload_with_context(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T20:57:27Z

tests/integration_tests/databases/commands/csv_upload_test.py

-    with app.app_context():
-        yield from _setup_csv_upload(["public"])
+@pytest.fixture()
+def setup_csv_upload_with_context_schema(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T20:57:34Z

tests/integration_tests/databases/commands/excel_upload_test.py

-    with app.app_context():
-        yield from _setup_excel_upload()
+@pytest.fixture()
+def setup_excel_upload_with_context(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T20:57:39Z

tests/integration_tests/databases/commands/excel_upload_test.py

-    with app.app_context():
-        yield from _setup_excel_upload(["public"])
+@pytest.fixture()
+def setup_excel_upload_with_context_schema(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T21:06:25Z

tests/integration_tests/charts/api_tests.py

-        with app.app_context():
-            cache_manager.data_cache.clear()
-            yield
+    def clear_data_cache(self, app_context: AppContext):


Reuse existing fixture. That said I'm not entirely sure why it's needed given it's in a class derived from Flask-Testing's TestCase class.

john-bodley · 2024-04-20T21:56:53Z

tests/integration_tests/security/migrate_roles_tests.py

@@ -41,9 +41,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms):
        pvms = []
        for old_pvm, new_pvms in pvm_map.items():
            pvms.append(
-                security_manager.add_permission_view_menu(


Black formatting.

john-bodley · 2024-04-20T22:21:28Z

tests/integration_tests/fixtures/users.py

-        sqllab_role = (
-            db.session.query(Role).filter(Role.name == "sql_lab").one_or_none()
-        )
+def create_gamma_sqllab_no_data(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T22:21:37Z

tests/integration_tests/fixtures/public_role.py

-    with app.app_context():
-        app.config["PUBLIC_ROLE_LIKE"] = "Gamma"
-        security_manager.sync_role_definitions()
+def public_role_like_gamma(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T22:21:43Z

tests/integration_tests/fixtures/public_role.py

-    with app.app_context():
-        app.config["PUBLIC_ROLE_LIKE"] = "TestRole"
-        security_manager.sync_role_definitions()
+def public_role_like_test_role(app_context: AppContext):


Reuse existing fixture.

john-bodley · 2024-04-20T22:31:56Z

tests/integration_tests/reports/scheduler_tests.py

@@ -35,150 +35,144 @@ def owners(get_user) -> list[User]:
    return [get_user("admin")]


-@pytest.mark.usefixtures("owners")
+@pytest.mark.usefixtures("app_context")


Use existing fixture.

betodealmeida

Looks great! Left a comment about having a consistent way to pass the app context.

betodealmeida · 2024-04-22T17:58:31Z

tests/integration_tests/fixtures/tags.py

@@ -22,12 +22,12 @@


 @pytest.fixture
+@pytest.mark.usefixtures("app_context")


Why not pass app_context as an argument, like in other places, for consistency?

betodealmeida · 2024-04-22T17:59:28Z

tests/integration_tests/migrations/87d38ad83218_migrate_can_view_and_drill_permission__tests.py

@@ -34,28 +36,28 @@
 downgrade = migration_module.do_downgrade


+@pytest.mark.usefixtures("app_context")


Same here, should we standardize on either using usefixtures or passing app_context to the test function/fixture?

@betodealmeida I tried to standardize this. It seems that:

If you want to use a fixture in a test you can either pass it as an argument or via @pytest.mark.usefixtures if test functions do not directly need access to a fixture object. Personally I find the later more readable as otherwise when you're reading through the code it feels like a potentially unused argument.

Somewhat cryptic, but if you want to use a fixture in another fixture you have to pass it as an argument*.

Per here it states,

Warning: Note this mark has no effect in fixture functions. For example, this will not work as expected:

@pytest.mark.usefixtures("my_other_fixture") @pytest.fixture def my_fixture_that_sadly_wont_use_my_other_fixture(): ...

Currently this will not generate any error or warning, but this is intended to be handled by issue #3664.

Maybe sticking with one pattern throughout is cleaner though—passing it as a function argument. I'm open to suggestions.

I spoke with @michael-s-molina offline and it seems like he's supportive of the approach I've taken here so I'm going to go ahead and merge this. I think as a follow up it would be good to use the @pytest.mark.usefixtures decorator where possible.

pull-request-size bot added the size/S label Apr 20, 2024

john-bodley force-pushed the john-bodley-patch-1 branch from bedac16 to 9d5b35e Compare April 20, 2024 05:23

pull-request-size bot added size/XXL and removed size/S labels Apr 20, 2024

john-bodley changed the title ~~chore(tests): Remove problematic nested app context~~ chore(tests): Remove unnecessary/problematic app contexts Apr 20, 2024

john-bodley force-pushed the john-bodley-patch-1 branch 4 times, most recently from f2c6f2e to 5afbff9 Compare April 20, 2024 06:32