fix(mcp): fix dataset tool tests using incorrect calling pattern #36279
Conversation
Two tests in TestDatasetSortableColumns were failing because they were attempting to call list_datasets.fn() directly, which doesn't exist on the decorated function object. The @tool and @parse_request decorators don't expose a .fn attribute. Fixed by updating both tests to use the correct MCP client pattern: - test_list_datasets_with_valid_order_column - test_default_ordering Both tests now properly use async with Client(mcp_server) and client.call_tool() to invoke the list_datasets tool, matching the pattern used in all other MCP service tests.
aminghadersohi
force-pushed
the
fix/dataset-tool-tests
branch
from
59446fa to
6a384ed
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #54e06e
Actionable Suggestions - 2
-
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py - 2
- Incomplete test assertions for ordering parameters · Line 1221-1222
- Incomplete test assertions for default ordering · Line 1221-1222
Additional Suggestions - 4
-
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py - 4
-
Missing return type annotation · Line 1165-1165Function `test_list_datasets_with_valid_order_column` at line 1165 is missing a return type annotation. Add `-> None` to the function signature.
Code suggestion
@@ -1165,1 +1165,1 @@ - async def test_list_datasets_with_valid_order_column(self, mock_list, mcp_server): + async def test_list_datasets_with_valid_order_column(self, mock_list, mcp_server) -> None:
-
Missing return type annotation · Line 1188-1188Function `test_sortable_columns_in_docstring` at line 1188 is missing a return type annotation. Add `-> None` to the function signature.
Code suggestion
@@ -1188,1 +1188,1 @@ - def test_sortable_columns_in_docstring(self): + def test_sortable_columns_in_docstring(self) -> None:
-
Missing return type annotation · Line 1203-1203Function `test_default_ordering` at line 1203 is missing a return type annotation. Add `-> None` to the function signature.
Code suggestion
@@ -1203,1 +1203,1 @@ - async def test_default_ordering(self, mock_list, mcp_server): + async def test_default_ordering(self, mock_list, mcp_server) -> None:
-
Trailing comma missing in function call · Line 1169-1169Line 1169 is missing a trailing comma after the last argument. Add a trailing comma after `schema="main"` for consistency.
Code suggestion
@@ -1169,1 +1169,1 @@ - dataset_id=1, table_name="Test Dataset", schema="main" + dataset_id=1, table_name="Test Dataset", schema="main",
-
Review Details
-
Files reviewed - 1 · Commit Range:
59446fa..59446fa- tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # Verify the mock was called | ||
| mock_list.assert_called_once() |
There was a problem hiding this comment.
The test for valid order column does not verify that DatasetDAO.list is called with the correct order_column and order_direction parameters. This could allow regressions where the ordering logic fails without detection. Add assertions to check the call arguments match the expected values.
Code suggestion
Check the AI-generated fix before applying
| # Verify the mock was called | |
| mock_list.assert_called_once() | |
| # Verify the mock was called | |
| mock_list.assert_called_once() | |
| call_args = mock_list.call_args | |
| assert call_args.kwargs["order_column"] == "table_name" | |
| assert call_args.kwargs["order_direction"] == "asc" |
Code Review Run #54e06e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| # Verify the mock was called | ||
| mock_list.assert_called_once() |
There was a problem hiding this comment.
The test for default ordering does not verify that DatasetDAO.list is called with the correct default order_column and order_direction parameters. This could allow regressions where default ordering fails. Add assertions to check the call arguments use the expected defaults.
Code suggestion
Check the AI-generated fix before applying
| # Verify the mock was called | |
| mock_list.assert_called_once() | |
| # Verify the mock was called | |
| mock_list.assert_called_once() | |
| call_args = mock_list.call_args | |
| assert call_args.kwargs["order_column"] == "changed_on" | |
| assert call_args.kwargs["order_direction"] == "desc" |
Code Review Run #54e06e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36279 +/- ##
==========================================
+ Coverage 60.48% 67.99% +7.50%
==========================================
Files 1931 636 -1295
Lines 76236 46817 -29419
Branches 8568 5081 -3487
==========================================
- Hits 46114 31831 -14283
+ Misses 28017 13710 -14307
+ Partials 2105 1276 -829
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Two tests in
TestDatasetSortableColumnswere failing withAttributeError: 'function' object has no attribute 'fn'because they were trying to calllist_datasets.fn()directly.The
@tooland@parse_requestdecorators don't expose a.fnattribute on the decorated function. The correct way to test MCP tools is to use the FastMCPClientpattern.Fixed tests:
test_list_datasets_with_valid_order_column- tests list_datasets with valid sortable columntest_default_ordering- tests default ordering behavior when no order_column is specifiedBoth tests now use
async with Client(mcp_server)andclient.call_tool()to properly invoke the MCP tool, matching the pattern used in all other MCP service tests.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - test fix only
TESTING INSTRUCTIONS
Run the failing tests before fix:
Both should fail with
AttributeError: 'function' object has no attribute 'fn'Run all MCP service tests to verify fix and no regressions:
All 240 tests should pass.
ADDITIONAL INFORMATION