shahar1 commented on code in PR #76265:
URL: https://github.com/apache/airflow/pull/76265#discussion_r4491431957
##########
airflow-ctl/src/airflowctl/ctl/commands/dag_command.py:
##########
@@ -116,7 +120,7 @@ def next_execution(args, api_client=NEW_API_CLIENT) -> dict
| None:
try:
response = api_client.dags.get(dag_id=args.dag_id)
except ServerResponseError as e:
- rich.print(f"[red]Error retrieving Dag {args.dag_id}: {e}[/red]")
+ rich.print(f"[red]Error retrieving Dag {escape(args.dag_id)}:
{escape(str(e))}[/red]")
Review Comment:
**minor** — [AGENTS.md, Testing
Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards):
"Every changed or added behaviour must have a test; every test must fail
without the PR's change."
Removing the escapes from each of these leaves all 496 airflow-ctl CLI tests
passing:
- this `next_execution` message (`dag_command.py:123`)
- `states_for_dag_run` (`task_command.py:127`)
- the logical-date not-found message (`dag_run.py:79`)
- `escape(dag_id)` in `update_dag_state` (`dag_command.py:60`)
- `escape(repr(dag_id))` in `_get_dag_run_by_run_id` (`dag_run.py:49`)
The first three need a case each, along the lines of
`test_get_by_run_id_404_prints_run_id_containing_markup`. The two `dag_id`
escapes can either get a case or be covered by putting a tag-like fragment in
the Dag id of an existing new test.
##########
airflow-ctl/tests/airflow_ctl/ctl/commands/test_auth_command.py:
##########
@@ -647,3 +647,53 @@ def
test_list_envs_environment_name_with_json_substring(self, monkeypatch):
auth_command.list_envs(args)
mock_get_password.assert_called_once_with("airflowctl",
"api_token_prod.json.region")
+
+
+def test_login_prints_failure_containing_markup(capsys):
+ api_client = mock.MagicMock()
Review Comment:
**minor** — [code review checklist, Testing
Requirements](https://github.com/apache/airflow/blob/main/.github/instructions/code-review.instructions.md#testing-requirements):
"**Flag any `mock.Mock()` or `mock.MagicMock()` without `spec` or
`autospec`.** Unspec'd mocks silently accept any attribute access, hiding real
bugs."
This applies to the five new `mock.MagicMock()` clients: here,
`test_auth_command.py:678`, `test_config_command.py:618`,
`test_connections_command.py:340` and `test_task_command.py:1059`. All of them
pass with `mock.MagicMock(spec=Client)`, which `test_connections_command.py`
and `test_dag_run.py` already use. The auth, config and task test files need
the `Client` import.
--
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]
