test(mcp): assert stored tool_filter in McpToolset init test#6327
Open
anxkhn wants to merge 1 commit into
Open
test(mcp): assert stored tool_filter in McpToolset init test#6327anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
test_init_with_tool_filter_list asserted `toolset._is_tool_selected is not None`, but `_is_tool_selected` is a method inherited from BaseToolset, so it is a bound method that is always non-None on every instance regardless of whether tool_filter was retained. The assertion could never fail and never checked what the test name promises: that the list filter is stored. A regression that dropped or mis-stored tool_filter in __init__ would still pass. Assert the stored value directly (`toolset.tool_filter == tool_filter`), matching the sibling init tests that check `toolset._connection_params == params`. The get_tools list-filtering behavior remains covered by test_get_tools_with_list_filter. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
test_init_with_tool_filter_listintests/unittests/tools/mcp_tool/test_mcp_toolset.pyis meant to check that alist
tool_filterpassed toMcpToolset(...)is retained, but its onlyassertion is:
_is_tool_selectedis a regular method inherited fromBaseToolset(
src/google/adk/tools/base_toolset.py), so on any instance it is a bound methodthat is always non-
None, independent of thetool_filtervalue. The assertioncan never fail and never checks what the test name promises: that the list filter
was stored. A regression that dropped or mis-stored
tool_filterin__init__would still pass green. The test's own comment even noted the intended
verification ("checking the filtering behavior in get_tools") was never written
here.
Solution:
Assert the stored value directly:
tool_filteris stored as a public attribute inBaseToolset.__init__(
self.tool_filter = tool_filter) and forwarded byMcpToolset.__init__viasuper().__init__(...), so this is a valid, non-tautological check. It mirrorsthe sibling init tests in the same file, which assert
toolset._connection_params == <params>. The actualget_toolslist-filteringbehavior is already covered separately by
test_get_tools_with_list_filter, sono behavioral coverage is lost. The stale two-line comment is replaced with a
single accurate one. Production code is untouched.
Testing Plan
Unit Tests:
Effectiveness (red -> green) was verified by temporarily setting
self.tool_filter = NoneinBaseToolset.__init__to simulate a dropped filter:the old assertion still passed (confirming it was vacuous), while the new
assertion failed with
AssertionError: assert None == ['tool1', 'tool2']. Thetemporary production change was reverted; this PR contains only the test change.
pytestsummary (local):Manual End-to-End (E2E) Tests:
Not applicable. This is a unit-test-only change with no runtime or user-facing
behavior change.
Checklist
Additional context
Diff is +2/-3 in a single file (
tests/unittests/tools/mcp_tool/test_mcp_toolset.py);no production code is changed.
pre-commit(isort, pyink, addlicense, ADKCompliance Checks) passes on the changed file.