vandonr-amz commented on code in PR #33423:
URL: https://github.com/apache/airflow/pull/33423#discussion_r1296464825
##########
tests/cli/conftest.py:
##########
@@ -34,17 +36,14 @@
custom_executor_module.CustomCeleryKubernetesExecutor = type( # type: ignore
"CustomCeleryKubernetesExecutor",
(celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
)
-custom_executor_module.CustomCeleryExecutor = type( # type: ignore
- "CustomLocalExecutor", (celery_executor.CeleryExecutor,), {}
-)
-custom_executor_module.CustomCeleryKubernetesExecutor = type( # type: ignore
- "CustomLocalKubernetesExecutor",
(celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomLocalExecutor = type( # type: ignore
+ "CustomLocalExecutor", (local_executor.LocalExecutor,), {}
)
-custom_executor_module.CustomCeleryExecutor = type( # type: ignore
- "CustomKubernetesExecutor", (celery_executor.CeleryExecutor,), {}
+custom_executor_module.CustomLocalKubernetesExecutor = type( # type: ignore
+ "CustomLocalKubernetesExecutor",
(local_kubernetes_executor.LocalKubernetesExecutor,), {}
)
-custom_executor_module.CustomCeleryKubernetesExecutor = type( # type: ignore
- "CustomCeleryKubernetesExecutor",
(celery_kubernetes_executor.CeleryKubernetesExecutor,), {}
+custom_executor_module.CustomKubernetesExecutor = type( # type: ignore
+ "CustomKubernetesExecutor", (kubernetes_executor.KubernetesExecutor,), {}
Review Comment:
this whole thing was a mess of copy-paste mistakes. Tests were passing only
because commands were never removed.
##########
tests/cli/test_cli_parser.py:
##########
@@ -205,57 +230,38 @@ def test_positive_int(self):
cli_config.positive_int(allow_zero=True)("-1")
@pytest.mark.parametrize(
- "executor",
+ "command",
[
"celery",
"kubernetes",
],
)
- def test_dag_parser_require_celery_executor(self, executor):
+ def test_executor_specific_commands_not_accessible(self, command):
with conf_vars({("core", "executor"): "SequentialExecutor"}),
contextlib.redirect_stderr(
io.StringIO()
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
with pytest.raises(SystemExit):
- parser.parse_args([executor])
- stderr = stderr.getvalue()
- assert (f"airflow command error: argument GROUP_OR_COMMAND: invalid
choice: '{executor}'") in stderr
-
- @pytest.mark.parametrize(
- "executor",
- [
- "CeleryExecutor",
- "CeleryKubernetesExecutor",
- "custom_executor.CustomCeleryExecutor",
- "custom_executor.CustomCeleryKubernetesExecutor",
- ],
- )
- def test_dag_parser_celery_command_accept_celery_executor(self, executor):
Review Comment:
this test was originally checking that celery executors were accepting the
celery sub-command, but this is now included in the parameterized test below,
with the kubernetes command at the same time.
##########
tests/cli/test_cli_parser.py:
##########
@@ -266,20 +272,12 @@ def test_cli_parser_executors(self, executor,
expected_args):
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
- with pytest.raises(SystemExit) as e:
+ with pytest.raises(SystemExit) as e: # running the help
command exits, so we prevent that
parser.parse_args([expected_arg, "--help"])
- assert e.value.code == 0
Review Comment:
this assert was never executed because it was in the block that captures the
exception
##########
tests/cli/test_cli_parser.py:
##########
@@ -266,20 +272,12 @@ def test_cli_parser_executors(self, executor,
expected_args):
) as stderr:
reload(cli_parser)
parser = cli_parser.get_parser()
- with pytest.raises(SystemExit) as e:
+ with pytest.raises(SystemExit) as e: # running the help
command exits, so we prevent that
parser.parse_args([expected_arg, "--help"])
- assert e.value.code == 0
+ assert e.value.code == 0, stderr.getvalue() # return code 0
== no problem
stderr = stderr.getvalue()
assert "airflow command error" not in stderr
- def test_dag_parser_config_command_dont_required_celery_executor(self):
Review Comment:
this was a obsolete test remaining from when there was code to check the
executor when executing a command. It was introduced after a bugfix on that
code (#17071), but the tested code has been removed since, so it's not testing
anything anymore.
##########
airflow/cli/cli_parser.py:
##########
@@ -41,11 +42,12 @@
GroupCommand,
core_commands,
)
+from airflow.cli.utils import CliConflictError
from airflow.exceptions import AirflowException
from airflow.executors.executor_loader import ExecutorLoader
from airflow.utils.helpers import partition
-airflow_commands = core_commands
+airflow_commands = core_commands.copy() # make a copy to prevent bad
interactions in tests
Review Comment:
since we do `airflow_commands.extend(...)` after that, and
`airflow_commands` is just pointing to `core_commands`, we were actually
modifying `core_commands`, and whatever we were adding in there stayed there
for all tests.
--
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]
