-
Notifications
You must be signed in to change notification settings - Fork 1
Fix/camelize for structs #15
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
Conversation
…ally, if we have structs we're not camlizing.
@oriolgual may I add in this PR the change suggested here to be more Ruby idiomatic? |
spec/lib/ai/mastra_client_spec.rb
Outdated
expect(WebMock).to( | ||
have_requested(:post, "https://mastra.local.factorial.dev/api/agents/marvin/generate").with do |req| | ||
body = JSON.parse(req.body) | ||
expect(body['telemetry']).to include('isEnabled' => false, 'recordInputs' => true, 'functionId' => 'test') | ||
expect(body['telemetry']).not_to have_key('is_enabled') | ||
end | ||
) |
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.
Can we use a webmock expectation instead of mocking webmock?
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.
We prefer directly the webmock manually even though it takes precedence over the VCR? We won't have a cassette with this:
stub = stub_request(:post, "https://mastra.local.factorial.dev/api/agents/marvin/generate")
.with do |req|
body = JSON.parse(req.body)
body['telemetry'] &&
body['telemetry']['isEnabled'] == false &&
body['telemetry']['recordInputs'] == true &&
body['telemetry']['functionId'] == 'test' &&
!body['telemetry'].key?('is_enabled') &&
!body['telemetry'].key?('record_inputs') &&
!body['telemetry'].key?('function_id')
end
.to_return(
status: 200,
body: { text: 'Test response' }.to_json,
headers: { 'Content-Type' => 'application/json' }
)
client.generate('marvin', messages: [Ai.user_message('test')], options: { telemetry: telemetry_settings })
expect(stub).to have_been_requested
But the test goes through the same way. Just asking to understand why, for the next time.
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.
I haven't find any other better way of having a test that performs a expect on the request, considering that VCR uses webmock behind scenes.
The closest hack I found would be to add the inspection of the cassette generated path, on the generated json, parse it, and verify the structure of that json.
I think my approach covers it in a more solid way, despite it looks weird to mock a mock, wdyt? @oriolgual
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.
But why do we need to hack things? Either we use VCR or we use WebMock, but I'm having a hard time understanding why mocking webmock is even necessary.
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.
Exactly my point yup, I don't want to hack things.
I'm not mocking a mock, I'm asserting on the request that VCR intercepts using WebMock, since VCR uses WebMock underneath.
VCR doesn’t provide a way to validate request payloads ( AFAIK ), and I want to ensure the struct-to-camelCase transformation works at the HTTP layer, not just in isolated unit logic.
Using expect(WebMock).to have_requested(...).with { ... }
lets me do that directly and clearly, without needing to parse cassettes or write indirect assertions.
The alternative, is the code I provided in the previous comment, where no cassette will be generated.
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.
Sorry I didn't properly read the previous message. Yes, I'd very much rather use a stub-request than asserting on whatever Webmock receives. The reasoning is that one feels normal and expected, and the other seems just brittle since VCR happens to use Webmock.
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.
I get the point. My only concern is that the trade off of that, is that we lack of something that validates we have run the test after these changes.
But I guess that's the same behavior if we don't update our cassettes 🤔
Will apply this reasoning in the future myself, probably you will get a cross-mention of mine just to make sure it's making sense there where I'm doing it too. Thanks!
…and solve it serializing it automatically as json, so the client doesn't have to worry about it, it will happen as soon as we transform to json to camelize
🚪 Why?
This Pull Request solves an issue where struct keys were not being properly converted to camelCase. This caused inconsistencies when interacting with Mastra API expecting camelCase keys.
This made that the telemetry settings where we need to ensure full privacy for the inputs and outputs, where not working.
🔑 What?
This PR updates the code responsible for serializing structs so that their keys are correctly converted to camelCase format.