-
Notifications
You must be signed in to change notification settings - Fork 57
Refactor multi search specs #406
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
ellnix
commented
Mar 13, 2025
- These tests were particularly slow, this is about a 50% reduction in time to complete (as reported by rspec).
- In addition refactored to clean up deprecation warnings from test output
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 89.54% 89.54%
=======================================
Files 13 13
Lines 775 775
=======================================
Hits 694 694
Misses 81 81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
+cc @wesharper if you would like to review. |
1e74e30 to
8b4e067
Compare
|
Looks good! I like the |
| Product.create! name: 'palm pixi plus', href: 'ebay', tags: ['terrible'] | ||
| Product.create! name: 'lg vortex', href: 'ebay', tags: ['decent'] | ||
| Product.create! name: 'palmpre', href: 'ebay', tags: ['discontinued', 'worst phone ever'] | ||
| Product.insert_all([ |
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.
Inserts multiple records into the database in a single SQL INSERT statement. It does not instantiate any models nor does it trigger Active Record callbacks or validations. Though passed values go through Active Record’s type casting and serialization.
Interesting that you create the records then you reindex the model, very clever!
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.
Thanks!
It's a method used very often in seed files, you can imagine that with a lot more records it would be a non-trivial amount of memory and computation to instantiate a ActiveRecord object for all of them.
I'm not really very happy with the solution here, I think it's a little bit undercooked, but I'm saving any refactoring for the inevitable PR where I probably make every test file into an rspec behavior so that I can apply it to every storage backend (AR, Sequel, Mongo).
|
bors merge |