shahar1 commented on code in PR #76651:
URL: https://github.com/apache/airflow/pull/76651#discussion_r4500810378
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -3676,6 +3676,23 @@ def
test_collect_results_processes_remaining_files_when_one_persist_fails(
assert manager._file_stats[file_b].run_count == 2
assert len(manager._processors) == 0
+ def test_collect_results_decrements_processes_metric(self):
+ manager = DagFileProcessorManager(max_runs=1)
+ file = DagFileInfo(bundle_name="testing", rel_path=Path("a.py"),
bundle_path=TEST_DAGS_FOLDER)
+ manager._file_stats[file] = DagFileStat()
+ proc, _ = self.mock_processor(start_time=time.monotonic() - 1)
+ proc.had_callbacks = True
+ manager._processors[file] = proc
+
+ with mock.patch("airflow.dag_processing.manager.stats.decr") as
stats_decr_mock:
Review Comment:
**minor**
From `.github/instructions/code-review.instructions.md`:
> **Flag any changed or added behaviour without a corresponding test, and
flag tests that a reviewer cannot fail by reverting the PR's change.**
This test runs with multi-team off, so `_get_team_names` returns `{}` and
the tag is pruned. Removing `"team_name"` from the new `decr` leaves it green.
The start, timeout, stop and terminate paths each have a multi-team test in
`TestMultiTeamMetrics`, and `_collect_results` has none.
Please move this test into that class and parametrize it like
`test_terminate_includes_team_name`. That also moves the `stats.decr` patch to
a decorator, which `AGENTS.md` prefers over `with mock.patch(...)`. The
`mock_processor` helper in that class returns only the process, not a tuple.
--
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]
