-
Notifications
You must be signed in to change notification settings - Fork 26
Test example code to ensure it runs #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I think the concept is solid. In this case, having the commits split makes it harder to follow later, so can you squash them into a single commit? |
skip "FIXME: this next code snippet is invalid and there doesn't seem to be a way to make both pass..." | ||
|
||
assert_json_lines( | ||
[ | ||
{ jsonrpc: "2.0", id: "1", result: { content: [{ type: "text", text: "OK" }], isError: false } }, | ||
], | ||
run_code_snippet("tool_definition_with_block"), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this is the relevant snippet.
tool = MCP::Tool.define( name: "my_tool", description: "This tool performs specific functionality...", annotations: { title: "My Tool", read_only_hint: true } ) do |args, server_context| Tool::Response.new([{ type: "text", text: "OK" }]) end
It fails with the following error
FAIL ReadmeCodeSnippetsTest#test_Tools_examples_work_exactly_as_documented_in_README (1.66s)
Expected no stderr in: Exception: #<MCP::Server::RequestHandlerError: Internal error calling tool my_tool>
Exception cause: #<ArgumentError: wrong number of arguments (given 1, expected 2)>
.
Expected "Exception: #<MCP::Server::RequestHandlerError: Internal error calling tool my_tool>\n" +
"Exception cause: #<ArgumentError: wrong number of arguments (given 1, expected 2)>\n"
to be empty.
test/integration/readme_code_snippets_test.rb:312:in 'block (2 levels) in ReadmeCodeSnippetsTest#run_code_snippet'
test/integration/readme_code_snippets_test.rb:306:in 'Dir.chdir'
test/integration/readme_code_snippets_test.rb:306:in 'block in ReadmeCodeSnippetsTest#run_code_snippet'
/opt/rubies/3.4.4/lib/ruby/3.4.0/tmpdir.rb:105:in 'Dir.mktmpdir'
test/integration/readme_code_snippets_test.rb:305:in 'ReadmeCodeSnippetsTest#run_code_snippet'
test/integration/readme_code_snippets_test.rb:154:in 'block in <class:ReadmeCodeSnippetsTest>'
I tried simply switching to a server_context:
keyword block argument, but that also fails
FAIL ReadmeCodeSnippetsTest#test_Tools_examples_work_exactly_as_documented_in_README (2.13s)
Expected no stderr in: Exception: #<MCP::Server::RequestHandlerError: Internal error calling tool my_tool>
Exception cause: #<ArgumentError: wrong number of arguments (given 0, expected 1; required keyword: server_context)>
.
Expected "Exception: #<MCP::Server::RequestHandlerError: Internal error calling tool my_tool>\n" +
"Exception cause: #<ArgumentError: wrong number of arguments (given 0, expected 1; required keyword: server_context)>\n"
to be empty.
test/integration/readme_code_snippets_test.rb:312:in 'block (2 levels) in ReadmeCodeSnippetsTest#run_code_snippet'
test/integration/readme_code_snippets_test.rb:306:in 'Dir.chdir'
test/integration/readme_code_snippets_test.rb:306:in 'block in ReadmeCodeSnippetsTest#run_code_snippet'
/opt/rubies/3.4.4/lib/ruby/3.4.0/tmpdir.rb:105:in 'Dir.mktmpdir'
test/integration/readme_code_snippets_test.rb:305:in 'ReadmeCodeSnippetsTest#run_code_snippet'
test/integration/readme_code_snippets_test.rb:154:in 'block in <class:ReadmeCodeSnippetsTest>'
Noticing #54, I suspect @julianojulio may have run into something similar.
test/fixtures/files/code_snippet_wrappers/readme/instrumentation_callback.rb
Outdated
Show resolved
Hide resolved
@koic I had kept the commits granular so it was clear which changes were required by which tests, in particular so reviewers can discuss them separately. I'm happy to squash after addressing review feedback though 👍 |
10ef613
to
7f2a867
Compare
|
||
assert_equal(<<~STDOUT, stdout) | ||
Bugsnag notified of #{error.inspect} with metadata #{metadata.inspect} | ||
Got instrumentation data #{instrumentation_data} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, because Ruby changed Hash#to_s
's output for Symbol
keys, we have to interpolate instead of inlining the value (so the test passes across Ruby versions), and because RuboCop detects we're interpolating a literal and inlines it, we have to extract a variable.
4c98005
to
7761f93
Compare
This adds sanity tests to ensure that example code in the `examples/` directory and `README.md` isn't outright wrong. These are not intended to serve as unit tests for the example functionality, just as smoke tests to catch things like API changes that require updating examples. Including a sanity test ensures that the example code isn't outright wrong. This is not intended to serve as unit tests for the example functionality. Some minor changes are included which facilitate this work: - Use `console` instead of `bash` codeblock language `console` highlights `$ ` prefixed lines differently from the following lines, which clearly distinguishes between commands and input/output. - Set `file_fixture_path` This allows us to use `ActiveSupport::TestCase#file_fixture`. - Add `ReadmeTestHelper` This helper provides utilities for extracting code snippets from `README.md`. Some of these tests revealed that either the examples were busted, or even bugs in the implementation. - Test README per-server configuration example This test revealed that the `define_` helper methods were failing to ensure the server supports the type of capability they were defining. - Test README protocol version examples This revealed that the `protocol_version` accessors weren't available on the `Server` at all, and the examples were incorrect. - Test README tool definition examples This revealed that the `define` example doesn't work, and the fix is unclear.
This PR introduces test files which run the code snippets in
/examples
and inREADME.md
. As each scenario is unique and requires its own setup and assertions, each test is bespoke (rather than being auto-generated).Motivation and Context
As part of #48 and the surrounding exploration of using simpler "definition" classes, I realized I'd have to ensure the example code still worked, so I thought I'd explore the idea of actually testing it.
It turns out this paid off, as a bunch of the examples were either broken themselves, or revealed bugs in the implementation.
How Has This Been Tested?
The PR consists almost entirely of tests. 😅
Breaking Changes
This should not include any breaking changes.
Types of changes
Checklist
Additional context
I have added
<!-- SNIPPET ID: snippet_name -->
comments to the snippets we need to be able to extract fromREADME.md
. These will be invisible to readers, but make it much simpler to find the snippets than needing to hard code their contents or some other such shenanigans.