test(tools): add unit tests for Bitbucket tools#1017
test(tools): add unit tests for Bitbucket tools#10177vignesh wants to merge 2 commits intoTracer-Cloud:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Greptile SummaryThis PR adds unit tests for the three Bitbucket tools ( Confidence Score: 5/5Safe to merge; all findings are P2 style/coverage suggestions that don't affect runtime correctness. All three issues found are P2: an unused import (lint-only), and two minor gaps in parametrize matrices for tests/tools/test_bitbucket_commits_tool.py — unused MagicMock import should be removed before merging to keep ruff clean. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_bb_available(sources)"] --> B{connection_verified?}
B -- No --> Z[False]
B -- Yes --> C{workspace?}
C -- No --> Z
C -- Yes --> D{username?}
D -- No --> Z
D -- Yes --> E{app_password?}
E -- No --> Z
E -- Yes --> F[True]
F --> G["BitbucketSearchCodeTool\n_search_bitbucket_code_available"]
F --> H["BitbucketCommitsTool\n_list_bitbucket_commits_available"]
F --> I["BitbucketFileContentsTool\n_get_bitbucket_file_contents_available"]
H --> J{repo_slug or repo?}
J -- No --> Z
J -- Yes --> K[True - Commits]
I --> L{repo_slug or repo?}
L -- No --> Z
L -- Yes --> M{path?}
M -- No --> Z
M -- Yes --> N[True - File Contents]
Reviews (1): Last reviewed commit: "test(tools): add Bitbucket tool unit tes..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from __future__ import annotations | ||
|
|
||
| from typing import Any, cast | ||
| from unittest.mock import MagicMock, patch |
There was a problem hiding this comment.
MagicMock import
MagicMock is imported but never referenced in this file — only patch is used. This will be flagged as F401 by ruff check, contradicting the PR's stated validation step.
| from unittest.mock import MagicMock, patch | |
| from unittest.mock import patch |
| @pytest.mark.parametrize( | ||
| "sources,expected", | ||
| [ | ||
| ( | ||
| { | ||
| "bitbucket": { | ||
| "connection_verified": True, | ||
| "workspace": "acme", | ||
| "username": "bb-user", | ||
| "app_password": "bb-pass", | ||
| "repo_slug": "backend-service", | ||
| } | ||
| }, | ||
| True, | ||
| ), | ||
| ({"bitbucket": {"connection_verified": True, "workspace": "acme"}}, False), | ||
| ({"bitbucket": {"connection_verified": True, "username": "bb-user"}}, False), | ||
| ({"bitbucket": {"connection_verified": True, "app_password": "bb-pass"}}, False), | ||
| ( | ||
| {"bitbucket": {"workspace": "acme", "username": "bb-user", "app_password": "bb-pass"}}, | ||
| False, | ||
| ), | ||
| ({}, False), | ||
| ], | ||
| ) | ||
| def test_is_available_requires_repo_and_credentials(sources: dict, expected: bool) -> None: | ||
| rt = _registered_tool() | ||
| assert rt.is_available(sources) is expected | ||
|
|
||
|
|
||
| def test_extract_params_maps_fields() -> None: |
There was a problem hiding this comment.
is_available negative case for absent repo_slug
_list_bitbucket_commits_available requires both the four credentials and a repo_slug (or repo) key. The parametrized matrix has no case where all four credentials are fully supplied but repo_slug is omitted — so the per-field credential tests don't prove that the repo_slug guard works independently of the credential guards. Adding a case like {"bitbucket": {"connection_verified": True, "workspace": "acme", "username": "bb-user", "app_password": "bb-pass"}} → False would complete the coverage.
| @pytest.mark.parametrize( | ||
| "sources,expected", | ||
| [ | ||
| ( | ||
| { | ||
| "bitbucket": { | ||
| "connection_verified": True, | ||
| "workspace": "acme", | ||
| "username": "bb-user", | ||
| "app_password": "bb-pass", | ||
| "repo_slug": "backend-service", | ||
| "path": "src/main.py", | ||
| } | ||
| }, | ||
| True, | ||
| ), | ||
| ({"bitbucket": {"connection_verified": True, "workspace": "acme"}}, False), | ||
| ({"bitbucket": {"connection_verified": True, "repo_slug": "backend-service"}}, False), | ||
| ({"bitbucket": {"connection_verified": True, "path": "src/main.py"}}, False), | ||
| ({}, False), | ||
| ], | ||
| ) | ||
| def test_is_available_requires_file_and_credentials(sources: dict, expected: bool) -> None: | ||
| rt = _registered_tool() | ||
| assert rt.is_available(sources) is expected |
There was a problem hiding this comment.
is_available negative cases for credential-present-but-field-absent combinations
_get_bitbucket_file_contents_available requires credentials and both repo_slug and path. The current matrix only has single-field cases (workspace alone, repo_slug alone, path alone), so there's no case proving the function returns False when credentials are fully satisfied but path is absent, or credentials + path are present but repo_slug is absent. Adding those two cases would fully verify the compound guard.
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage for the Bitbucket tool entrypoints and tightens Bitbucket tool availability so tools are only considered available when verified credentials are present in sources["bitbucket"].
Changes:
- Add
BaseToolContract-style unit tests for Bitbucket commits, file contents, and search code tools. - Expand Bitbucket availability helper (
_bb_available) to requireconnection_verifiedplusworkspace,username, andapp_password.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/tools/test_bitbucket_search_code_tool.py |
Adds contract/is_available/extract_params/run tests for search_bitbucket_code. |
tests/tools/test_bitbucket_file_contents_tool.py |
Adds contract/is_available/extract_params/run tests for get_bitbucket_file_contents. |
tests/tools/test_bitbucket_commits_tool.py |
Adds contract/is_available/extract_params/run tests for list_bitbucket_commits. |
app/tools/BitbucketSearchCodeTool/__init__.py |
Updates shared _bb_available helper to require verified, complete Bitbucket credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_run_returns_unavailable_without_credentials() -> None: | ||
| result = search_bitbucket_code(query="error OR exception") | ||
|
|
||
| assert result["available"] is False | ||
| assert result["results"] == [] | ||
| assert result["error"] == "Bitbucket integration is not configured." |
There was a problem hiding this comment.
The missing-credentials test relies on the real environment having no BITBUCKET_* variables. If BITBUCKET_WORKSPACE (and/or credentials) are set locally/CI, _resolve_config() may return a non-None config, causing this test to either make a real network call or return a different shape (e.g., error: "Not configured."). Patch app.tools.BitbucketSearchCodeTool.bitbucket_config_from_env to return None in this test to keep it deterministic.
| def test_run_returns_unavailable_without_credentials() -> None: | ||
| result = get_bitbucket_file_contents(repo_slug="backend-service", path="src/main.py") | ||
|
|
||
| assert result["available"] is False | ||
| assert result["file"] == {} | ||
| assert result["error"] == "Bitbucket integration is not configured." |
There was a problem hiding this comment.
This missing-credentials test can become flaky if BITBUCKET_* env vars are present, because _resolve_config() may load env config and the tool may call the real Bitbucket integration. Patch app.tools.BitbucketSearchCodeTool.bitbucket_config_from_env to return None here so the test always exercises the unavailable path without side effects.
| def test_run_returns_unavailable_without_credentials() -> None: | ||
| result = list_bitbucket_commits(repo_slug="backend-service") | ||
|
|
||
| assert result["available"] is False | ||
| assert result["commits"] == [] | ||
| assert result["error"] == "Bitbucket integration is not configured." |
There was a problem hiding this comment.
This missing-credentials test assumes there is no Bitbucket env configuration. If BITBUCKET_WORKSPACE/credentials are set, _resolve_config() can return a config and the tool may call the real Bitbucket API (or return a different error payload). Patch app.tools.BitbucketSearchCodeTool.bitbucket_config_from_env to return None in this test to make it deterministic and side-effect free.
…ra availability cases; remove unused import
|
@yashksaini-coder |
Fixes #992
Added unit tests for the Bitbucket tool entrypoints:
BitbucketCommitsToolBitbucketFileContentsToolBitbucketSearchCodeToolThis PR covers:
is_availablechecks for verified Bitbucket credentialsextract_paramsmapping for repo, path, query, and credentialsrunhappy-path behaviorI also updated the shared Bitbucket availability helper so the tools require verified Bitbucket credentials, not just a present Bitbucket source.
Demo/Screenshot for feature changes and bug fixes -
Validation output:
python -m pytest tests/tools/test_bitbucket_commits_tool.py tests/tools/test_bitbucket_file_contents_tool.py tests/tools/test_bitbucket_search_code_tool.py -qpython -m ruff check app/ tests/python -m ruff format --check app/ tests/python -m mypy app/Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Added focused unit tests for each Bitbucket tool using the existing
BaseToolContractpattern. The tests validate the tool metadata contract, source availability, parameter extraction, happy-path execution, and unavailable-path behavior. I used direct function patching against the Bitbucket integration helpers so the tests stay deterministic and do not make network calls.Checklist before requesting a review