Copilot commented on code in PR #64942:
URL: https://github.com/apache/airflow/pull/64942#discussion_r3066476111
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -1453,6 +1453,30 @@ def test_add_callback_skips_when_bundle_init_fails(self,
mock_bundle_manager):
bundle.initialize.assert_called_once()
assert len(manager._callback_to_execute) == 0
+ @mock.patch("airflow.dag_processing.manager.DagBundlesManager")
+ def
test_add_callback_initializes_versioned_bundle_when_version_is_none(self,
mock_bundle_manager):
+ """Ensure versioned bundles are initialized even when bundle_version
is None."""
+ manager = DagFileProcessorManager(max_runs=1)
+ bundle = MagicMock(spec=BaseDagBundle)
+ bundle.supports_versioning = True
+ bundle.path = Path("/tmp/bundle")
Review Comment:
This test primarily asserts an internal interaction (`initialize()` called)
rather than the user-visible behavior that motivated the bug (callbacks should
be queued with a materialized `bundle.path`). To better validate the
regression, set `bundle.path` to `None` initially and make
`bundle.initialize()` populate it (e.g., via a side effect), then assert the
queued `DagFileInfo` has a non-None `bundle_path` (and/or matches the expected
path). This keeps the test tied to the observable outcome instead of the
specific control-flow.
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -1453,6 +1453,30 @@ def test_add_callback_skips_when_bundle_init_fails(self,
mock_bundle_manager):
bundle.initialize.assert_called_once()
assert len(manager._callback_to_execute) == 0
+ @mock.patch("airflow.dag_processing.manager.DagBundlesManager")
+ def
test_add_callback_initializes_versioned_bundle_when_version_is_none(self,
mock_bundle_manager):
+ """Ensure versioned bundles are initialized even when bundle_version
is None."""
+ manager = DagFileProcessorManager(max_runs=1)
+ bundle = MagicMock(spec=BaseDagBundle)
+ bundle.supports_versioning = True
+ bundle.path = Path("/tmp/bundle")
Review Comment:
Using a hard-coded `/tmp/...` path makes the test less portable (and can be
problematic on non-POSIX environments). Prefer using pytest’s `tmp_path`
fixture (or `tempfile`) to create a temporary directory and set `bundle.path`
from that.
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -577,7 +577,7 @@ def _add_callback_to_queue(self, request: CallbackRequest):
# Bundle no longer configured
self.log.error("Bundle %s no longer configured, skipping
callback", request.bundle_name)
return None
- if bundle.supports_versioning and request.bundle_version:
+ if bundle.supports_versioning:
Review Comment:
Calling `bundle.initialize()` unconditionally for every callback on a
versioned bundle can be expensive (e.g., repeated tracking-repo refreshes) and
may introduce unnecessary load when many callbacks are queued. Consider
aligning with `_refresh_dag_bundles` by guarding with `if not
bundle.is_initialized:` (or an equivalent idempotency check) before
initializing, so initialization only happens when needed.
```suggestion
if bundle.supports_versioning and not bundle.is_initialized:
```
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -1453,6 +1453,30 @@ def test_add_callback_skips_when_bundle_init_fails(self,
mock_bundle_manager):
bundle.initialize.assert_called_once()
assert len(manager._callback_to_execute) == 0
+ @mock.patch("airflow.dag_processing.manager.DagBundlesManager")
+ def
test_add_callback_initializes_versioned_bundle_when_version_is_none(self,
mock_bundle_manager):
+ """Ensure versioned bundles are initialized even when bundle_version
is None."""
+ manager = DagFileProcessorManager(max_runs=1)
+ bundle = MagicMock(spec=BaseDagBundle)
+ bundle.supports_versioning = True
+ bundle.path = Path("/tmp/bundle")
+ mock_bundle_manager.return_value.get_bundle.return_value = bundle
+
+ request = DagCallbackRequest(
+ filepath="file1.py",
+ dag_id="dag1",
+ run_id="run1",
+ is_failure_callback=False,
+ bundle_name="testing",
+ bundle_version=None,
+ msg=None,
+ )
+
+ manager._add_callback_to_queue(request)
+
+ bundle.initialize.assert_called_once()
+ assert len(manager._callback_to_execute) == 1
Review Comment:
This test primarily asserts an internal interaction (`initialize()` called)
rather than the user-visible behavior that motivated the bug (callbacks should
be queued with a materialized `bundle.path`). To better validate the
regression, set `bundle.path` to `None` initially and make
`bundle.initialize()` populate it (e.g., via a side effect), then assert the
queued `DagFileInfo` has a non-None `bundle_path` (and/or matches the expected
path). This keeps the test tied to the observable outcome instead of the
specific control-flow.
--
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]
