-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor model and controller to follow Rails conventions #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
83f4e4a to
406644a
Compare
etagwerker
left a comment
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.
@arielj Sorry for the delay!
This looks good, but I'm getting an error when testing it locally:
Started POST "/reports" for ::1 at 2025-10-24 16:34:42 -0400
ActiveRecord::SchemaMigration Load (0.5ms) SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
Processing by ReportsController#create as */*
Parameters: {"{\"entries\":"=>{"{\"name\":\"Enumerable#reverse.detect\",\"central_tendency\":191701.4517993972,\"ips\":191701.4517993972,\"error\":5033,\"stddev\":5033,\"microseconds\":5043669.0,\"iterations\":966160,\"cycles\":18580},{\"name\":\"Enumerable#select.last\",\"central_tendency\":5433.2019560685785,\"ips\":5433.2019560685785,\"error\":88,\"stddev\":88,\"microseconds\":5044405.0,\"iterations\":27400,\"cycles\":548}"=>{",\"options\":{\"compare\":true}}"=>nil}}}
Completed 400 Bad Request in 4ms (ActiveRecord: 0.0ms (0 queries, 0 cached) | GC: 0.2ms)
What's concerning is that the test suite passes (when it probably shouldn't)
The same request works in main but it fails in this branch. Thoughts?
it looks like benchmark-ips is sending the request with no content_type, so rails parses the params incorrectly and ends up with I updated the code adding a test for this scenario and also code in the controller that updates the params object with the body correctly parsed in these cases for completeness, if we add |
@arielj This would be great. Could you please open a PR for that in the |
This PR includes different changes: