Copilot commented on code in PR #64879:
URL: https://github.com/apache/airflow/pull/64879#discussion_r3097460188
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +920,66 @@ 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
+ ):
+ """GIT_SSH_COMMAND from 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_repo = mock.MagicMock()
+ ssh_ctx = mock.MagicMock()
+ mock_repo.git.custom_environment.return_value = ssh_ctx
Review Comment:
These new tests create `MagicMock()` instances without a `spec`/`autospec`
(for both the repo and the context manager). Using a spec helps catch typos and
mismatched GitPython APIs (e.g. `git.custom_environment`, `git.submodule`) and
prevents silently passing tests when production code changes. Consider
switching these mocks to `create_autospec(...)` or `MagicMock(spec=...)`
targeting the minimal interface you use here.
##########
providers/git/tests/unit/git/bundles/test_git.py:
##########
@@ -920,6 +920,66 @@ 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
+ ):
+ """GIT_SSH_COMMAND from 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_repo = mock.MagicMock()
+ ssh_ctx = mock.MagicMock()
+ mock_repo.git.custom_environment.return_value = ssh_ctx
+
+ bundle = GitDagBundle(
+ name="test",
+ git_conn_id="git_default",
+ tracking_ref="main",
+ version="123456",
+ submodules=True,
+ )
+ bundle.repo = mock_repo
+
+ bundle._fetch_submodules()
+
+
mock_repo.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_repo.git.submodule.assert_has_calls(
+ [mock.call("sync", "--recursive"), mock.call("update", "--init",
"--recursive", "--jobs", "1")]
+ )
+ assert mock_repo.git.mock_calls[0] ==
mock.call.custom_environment(GIT_SSH_COMMAND=expected_ssh_cmd)
+ assert mock_repo.git.mock_calls[1] == mock.call.submodule("sync",
"--recursive")
+
+ @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 has no GIT_SSH_COMMAND, 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 = {}
+
+ mock_repo = mock.MagicMock()
+ bundle = GitDagBundle(
Review Comment:
This test uses an unspec'd `MagicMock()` for `bundle.repo`. Adding a
`spec`/`autospec` (even if only for the `git` attribute and its `submodule`
method) will make the test better at detecting API misuse and reduce the chance
of false positives.
--
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]
