Skip to content

Speed up tests #408

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Speed up tests #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d582b98
Make test reset helpers async
ellnix Mar 16, 2025
54e8edc
Speed up various tests
ellnix Mar 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions spec/model_methods_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@
it 'uses the specified scope' do
TestUtil.reset_colors!

Color.create!(name: 'red', short_name: 'r3', hex: 3)
Color.create!(name: 'red', short_name: 'r1', hex: 1)
Color.create!(name: 'purple', short_name: 'p')

Color.clear_index!(true)
Color.insert_all([
{ name: 'red', short_name: 'r3', hex: 3 },
{ name: 'red', short_name: 'r1', hex: 1 },
{ name: 'purple', short_name: 'p', hex: 4 }
])

Color.where(name: 'red').reindex!(3, true)
expect(Color.search('').size).to eq(2)

Color.clear_index!(true)
Color.where(id: Color.first.id).reindex!(3, true)
Color.clear_index!
Color.where(name: 'purple').reindex!(3, true)
expect(Color.search('').size).to eq(1)
end
end
Expand Down Expand Up @@ -66,6 +66,7 @@
)
end

# No need to await since Book is synchronous
expect(Book.search('Frankenstein')).to be_one
end
end
Expand All @@ -77,6 +78,7 @@
_blue = Color.create!(name: 'blue', short_name: 'blu', hex: 0x0000FF)
_black = Color.create!(name: 'black', short_name: 'bla', hex: 0x000000)

# No need to await since Color is synchronous
json = Color.raw_search('')
Color.index_documents Color.limit(1), true # reindex last color, `limit` is incompatible with the reindex! method
expect(json['hits'].count).to eq(Color.raw_search('')['hits'].count)
Expand Down
6 changes: 2 additions & 4 deletions spec/ms_clean_up_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
require 'spec_helper'
require 'support/models/restaurant'
require 'support/models/book'

RSpec.describe 'MeiliSearch::Rails::MSCleanUpJob' do
include ActiveJob::TestHelper
Expand All @@ -9,10 +11,6 @@ def clean_up_indexes

def create_indexed_record
record

indexes.each do |index|
index.wait_for_task(index.tasks['results'].last['uid'])
end
end

subject(:clean_up) { MeiliSearch::Rails::MSCleanUpJob }
Expand Down
5 changes: 2 additions & 3 deletions spec/safe_index_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
genres = %w[Legend Fiction Crime].cycle
authors = %w[A B C].cycle

5.times do
Book.create! name: Faker::Book.title, author: authors.next, genre: genres.next
end
Book.insert_all(Array.new(5) { { name: Faker::Book.title, author: authors.next, genre: genres.next } })
Book.reindex!

expect do
Book.index.facet_search('genre', 'Fic', filter: 'author = A')
Expand Down
4 changes: 2 additions & 2 deletions spec/support/models/animals.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ def ms_id

module TestUtil
def self.reset_animals!
Cat.clear_index!(true)
Cat.clear_index!
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly did the true do? Is this just a style cleanup or are there perf implications?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear_index! and a lot of other methods in this gem have a single argument that is a synchronous flag, i. e. whether to await the resulting task. This absolutely has performance implications (although not all that many since awaiting is not really avoidable).

I often write descriptions in git commits, here's the one on the commit of this change:

Unnecessary awaits should be avoided, generally these methods are meant
to be called at the beginning of the test and for best performance we
should wait on tasks right before an assertion.

So the performance implication is being lazy and putting off the await for as long as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also confusion like this is why I really don't like flags, consider this method signature:

      def ms_reindex!(batch_size = MeiliSearch::Rails::IndexSettings::DEFAULT_BATCH_SIZE, synchronous = false)

Not only is the first argument just a magic number (in most cases), but the second argument is a flag. And because we do not encourage the use of await, we have the flag come second so specifying only the flag takes this form:

Color.ms_reindex!(MeiliSearch::Rails::IndexSettings::DEFAULT_BATCH_SIZE, true)

I would argue this does not look great.

In concert with a specific batch size, you get this borderline incomprehensible method call:

Color.ms_reindex!(50, true)

In conclusion, I would really like to change these methods :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the writeup. Agreed on the method signature. kwargs are definitely preferred IMO as well.

Cat.delete_all
Dog.clear_index!(true)
Dog.clear_index!
Dog.delete_all
end
end
2 changes: 1 addition & 1 deletion spec/support/models/book.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def public?

module TestUtil
def self.reset_books!
Book.clear_index!(true)
Book.clear_index!
Book.index(safe_index_uid('BookAuthor')).delete_all_documents
Book.index(safe_index_uid('Book')).delete_all_documents
end
Expand Down
2 changes: 1 addition & 1 deletion spec/support/models/color.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def will_save_change_to_short_name?

module TestUtil
def self.reset_colors!
Color.clear_index!(true)
Color.clear_index!
Color.delete_all
end
end
2 changes: 1 addition & 1 deletion spec/support/models/movie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class Movie < ActiveRecord::Base

module TestUtil
def self.reset_movies!
Movie.clear_index!(true)
Movie.clear_index!
Movie.delete_all
end
end
2 changes: 1 addition & 1 deletion spec/support/models/people.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def will_save_change_to_full_name?

module TestUtil
def self.reset_people!
People.clear_index!(true)
People.clear_index!
People.delete_all
end
end
Loading