[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros

[GitBox <gi...@apache.org>]

RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535447889



##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
             assert "Failed to import plugin test-entrypoint" in received_logs
             assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
 
+    def test_registering_plugin_macros(self):
+        """
+        Tests whether macros that originate from plugins are being registered correctly.
+        """
+        from airflow import macros
+        from airflow.plugins_manager import integrate_macros_plugins
+
+        def custom_macro():
+            return 'foo'
+
+        class MacroPlugin(AirflowPlugin):
+            name = 'macro_plugin'
+            macros = [custom_macro]
+
+        with mock_plugin_manager(plugins=[MacroPlugin()]):
+            # Ensure the macros for the plugin have been integrated.
+            integrate_macros_plugins()
+            # Test whether the modules have been created as expected.
+            plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+            for macro in MacroPlugin.macros:
+                # Verify that the macros added by the plugin are being set correctly
+                # on the plugin's macro module.
+                assert hasattr(plugin_macros, macro.__name__)
+            # Verify that the symbol table in airflow.macros has been updated with an entry for
+            # this plugin, this is necessary in order to allow the plugin's macros to be used when
+            # rendering templates.
+            assert hasattr(macros, MacroPlugin.name)

Review comment:
       `integrate_macros_plugins()` has the side-effect that it modifies the `airflow.macros` module. Do we need to add additional cleanup logic to reset the contents of `airflow.macros` through a finalizer, here?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org