Copilot commented on code in PR #64879:
URL: https://github.com/apache/airflow/pull/64879#discussion_r3107884108
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +932,103 @@ def
test_clone_bare_repo_invalid_repository_error_retry_fails(
# Verify Repo was called twice (failed attempt + failed retry)
assert mock_repo_class.call_count == 2
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_fetch_submodules_uses_custom_environment_when_git_ssh_command_set(
+ self, mock_githook_class
+ ):
+ """Non-empty hook.env is passed to custom_environment; submodule runs
inside that context."""
+ expected_ssh_cmd = (
+ "ssh -i /path/key -o IdentitiesOnly=yes -o
UserKnownHostsFile=/path/known_hosts"
+ )
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = {"GIT_SSH_COMMAND": expected_ssh_cmd}
+
+ mock_git = mock.create_autospec(Git, instance=True)
+ ssh_ctx = mock.create_autospec(_GitCustomEnvContextManager,
instance=True)
+ mock_git.custom_environment.return_value = ssh_ctx
+ mock_repo = mock.MagicMock(spec=Repo)
+ mock_repo.git = mock_git
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+
mock_git.custom_environment.assert_called_once_with(GIT_SSH_COMMAND=expected_ssh_cmd)
+ ssh_ctx.__enter__.assert_called_once()
+ ssh_ctx.__exit__.assert_called_once()
+ mock_git.submodule.assert_has_calls(
+ [mock.call("sync", "--recursive"), mock.call("update", "--init",
"--recursive", "--jobs", "1")]
+ )
+
Review Comment:
This test’s docstring says the submodule commands run inside the
`custom_environment` context, but the assertions only check that
`__enter__`/`__exit__` were called and that `submodule(...)` was called—without
verifying ordering. Consider asserting call order (e.g., via a shared parent
mock or `attach_mock`) so the test will fail if the context manager is
entered/exited before/after the submodule calls rather than wrapping them.
```suggestion
parent = mock.Mock()
parent.attach_mock(mock_git.custom_environment, "custom_environment")
parent.attach_mock(ssh_ctx.__enter__, "enter")
parent.attach_mock(mock_git.submodule, "submodule")
parent.attach_mock(ssh_ctx.__exit__, "exit")
assert parent.mock_calls == [
mock.call.custom_environment(GIT_SSH_COMMAND=expected_ssh_cmd),
mock.call.enter(),
mock.call.submodule("sync", "--recursive"),
mock.call.submodule("update", "--init", "--recursive", "--jobs",
"1"),
mock.call.exit(None, None, None),
]
```
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +932,103 @@ def
test_clone_bare_repo_invalid_repository_error_retry_fails(
# Verify Repo was called twice (failed attempt + failed retry)
assert mock_repo_class.call_count == 2
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_fetch_submodules_uses_custom_environment_when_git_ssh_command_set(
+ self, mock_githook_class
+ ):
+ """Non-empty hook.env is passed to custom_environment; submodule runs
inside that context."""
+ expected_ssh_cmd = (
+ "ssh -i /path/key -o IdentitiesOnly=yes -o
UserKnownHostsFile=/path/known_hosts"
+ )
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = {"GIT_SSH_COMMAND": expected_ssh_cmd}
+
+ mock_git = mock.create_autospec(Git, instance=True)
+ ssh_ctx = mock.create_autospec(_GitCustomEnvContextManager,
instance=True)
+ mock_git.custom_environment.return_value = ssh_ctx
+ mock_repo = mock.MagicMock(spec=Repo)
+ mock_repo.git = mock_git
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+
mock_git.custom_environment.assert_called_once_with(GIT_SSH_COMMAND=expected_ssh_cmd)
+ ssh_ctx.__enter__.assert_called_once()
+ ssh_ctx.__exit__.assert_called_once()
+ mock_git.submodule.assert_has_calls(
+ [mock.call("sync", "--recursive"), mock.call("update", "--init",
"--recursive", "--jobs", "1")]
+ )
+
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def test_fetch_submodules_forwards_full_hook_env_including_passphrase_vars(
+ self, mock_githook_class
+ ):
+ """SSH_ASKPASS / DISPLAY etc. must reach git subprocesses, not only
GIT_SSH_COMMAND."""
+ full_hook_env = {
+ "GIT_SSH_COMMAND": "ssh -i /path/key -o IdentitiesOnly=yes",
+ "SSH_ASKPASS": "/tmp/airflow-git-askpass.sh",
+ "SSH_ASKPASS_REQUIRE": "force",
+ "DISPLAY": ":0",
+ }
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = full_hook_env
+
+ mock_git = mock.create_autospec(Git, instance=True)
+ ssh_ctx = mock.create_autospec(_GitCustomEnvContextManager,
instance=True)
+ mock_git.custom_environment.return_value = ssh_ctx
+ mock_repo = mock.MagicMock(spec=Repo)
+ mock_repo.git = mock_git
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+ mock_git.custom_environment.assert_called_once_with(**full_hook_env)
+
+ @mock.patch("airflow.providers.git.bundles.git.GitHook")
+ def
test_fetch_submodules_skips_custom_environment_without_git_ssh_command(self,
mock_githook_class):
+ """When hook.env is empty, submodule update does not use
custom_environment."""
+ mock_hook = mock_githook_class.return_value
+ mock_hook.repo_url = "[email protected]:apache/airflow.git"
+ mock_hook.env = {}
Review Comment:
The test name
`test_fetch_submodules_skips_custom_environment_without_git_ssh_command`
doesn’t match what the test actually verifies: it sets `hook.env = {}` and
asserts `custom_environment` is not called. In production code,
`custom_environment` is used whenever `hook.env` is a non-empty dict (even if
it didn’t contain `GIT_SSH_COMMAND`). Rename this test to reflect the empty-env
behavior, or change the fixture to use a non-empty env without
`GIT_SSH_COMMAND` and assert the intended behavior.
--
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]
