feat: Enhance tool descriptions #350
Conversation
DaleSeo
force-pushed
the
imporove-tool-descriptions
branch
from
eebbadb to
2d513b7
Compare
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: a1132a93b9c96d80cdbd1a02 URL: https://www.apollographql.com/docs/deploy-preview/a1132a93b9c96d80cdbd1a02 |
DaleSeo
force-pushed
the
imporove-tool-descriptions
branch
from
6feede6 to
7d57d16
Compare
DaleSeo
force-pushed
the
imporove-tool-descriptions
branch
from
7d57d16 to
b9c3b91
Compare
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_tool_description_non_minified(schema: Valid<Schema>) { |
There was a problem hiding this comment.
Can we use better test names are that a bit more descriptive? I like following this guide for test name conventions https://enterprisecraftsmanship.com/posts/you-naming-tests-wrong/
There was a problem hiding this comment.
Isn't the current test name already close to the conventions you linked? If you have any specific suggestions for renaming, just let me know and I'll update it.
There was a problem hiding this comment.
So the test is verifying that the description is not minified and doesn't contain the minified legend. I would use a test name that indicates this at first glance, maybe something like introspect_tool_description_is_not_minified. Not need to add "test" in the name IMO since thats a bit superfluous.
There was a problem hiding this comment.
@alocay I get your naming preference, but adding test_ as a prefix for test methods is a common practice in our codebase and might even be a company standard, like with Router. I'm concerned that removing it here could introduce inconsistency so I'll update it to test_introspect_tool_description_is_not_minified for now.
If you’d like to revisit this convention, I recommend proposing the change to the team and, if there's consensus, we can bulk update test names across the codebase to keep things consistent.
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_tool_description_non_minified(schema: Valid<Schema>) { |
There was a problem hiding this comment.
So the test is verifying that the description is not minified and doesn't contain the minified legend. I would use a test name that indicates this at first glance, maybe something like introspect_tool_description_is_not_minified. Not need to add "test" in the name IMO since thats a bit superfluous.
| // Should not contain minification legend | ||
| assert!(!description.contains("T=type,I=input")); | ||
| // Should mention conditional search tool usage | ||
| assert!(description.contains("If the search tool is also available")); |
There was a problem hiding this comment.
nit: you could argue that this last condition is testing something else entirely and not related to the minification stuff. You could technically split this in a separate test called like instrospect_description_mentions_search_tool_usage. I find it helpful to have small very directed unit tests. It helps quickly determining what is broken in tests in the future.
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_tool_description_minified(schema: Valid<Schema>) { |
There was a problem hiding this comment.
this name could be introspect_tool_description_is_minified_with_an_appropriate_legend or something
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn test_tool_description_non_minified(schema: Valid<Schema>) { |
There was a problem hiding this comment.
nit: ditto here and below, can use something like search_tool_description_is_not_minified.
| description | ||
| .contains("Search a GraphQL schema for types matching the provided search terms") | ||
| ); | ||
| assert!(description.contains("Instructions: If the introspect tool is also available")); |
There was a problem hiding this comment.
nit: these two asserts aren't related to minified stuff. You could split these two into a separate test like search _tool_description_contains_expected_instructions or something of that nature. That way if it breaks in the future we don't have debug to see if it's breaking due to the minification related asserts or these.
We received feedback from DevRel that the descriptions of the introspect and search MCP tools are too simple for AI models to identify the intended tools and optimize the context window. This PR enhances the descriptions of these tools to offer clearer guidance for AI models on efficient GraphQL schema exploration patterns.