Split `spec/integration_spec.rb` #350

ellnix · 2024-02-29T19:18:55Z

~~Continues after #345: Please only look at commits after "Move Post specs"~~
More changes toward resolving #332

Fixes #358

This test seems to just use Marshal.dump on an object and then Marshal.load it later, which would pass for any ruby object.

This test seems to test that an object that is cached can be fetched back from the cache. As proof, the meilisearch method calls can be removed and the test passes just fine: ```ruby context 'with Rails caching' do let(:memory_store) { ActiveSupport::Cache.lookup_store(:memory_store) } let(:search_query) { '*' } let(:cache_key) { "book_search:#{search_query}" } before do allow(Rails).to receive(:cache).and_return(memory_store) Rails.cache.clear end fit 'caches the search results' do # Ensure the cache is empty before the test expect(Rails.cache.read(cache_key)).to be_nil # Perform the search and cache the results Rails.cache.fetch(cache_key) do search_query end # Check if the search result is cached expect(Rails.cache.read(cache_key)).to eq(search_query) end end ```

It does not seem like Mongoid::Document has bang methods, so this is spec is not reproducing a likely naming conflict. In either case, the plan is to test with real MongoDB models so this test would be removed eventually.

'is_synchronous' tests a private method and only tests that the configuration was applied 'has maxValuesPerFacet set' tests meilisearch-ruby's Index class

codecov · 2024-02-29T19:20:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.90%. Comparing base (40a6abb) to head (487c780).
Report is 45 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
- Coverage   89.29%   88.90%   -0.40%     
==========================================
  Files          13       13              
  Lines         757      757              
==========================================
- Hits          676      673       -3     
- Misses         81       84       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ellnix · 2024-03-01T13:38:47Z

spec/integration_spec.rb

@@ -158,10 +158,6 @@
    expect(Color.search('').size).to eq(1)
  end

-  it 'has a Rails env-based index name' do


Moved to configuration_spec.rb, seemed more appropriate.

ellnix · 2024-03-01T13:39:25Z

spec/integration_spec.rb

-    Book.index(safe_index_uid('Book')).delete_all_documents
-  end
-
-  it 'returns array of tasks on #ms_index!' do


Moved to instance_methods_spec.rb since it concerns the interface (return type) of one of the instance methods we add to models.

ellnix · 2024-03-01T13:40:32Z

spec/integration_spec.rb

-    )
-  end
-
-  it 'indexes the book in 2 indexes of 3' do


Moved to options_spec.rb since it tests whether or not the :if option is behaving correctly.

ellnix · 2024-03-01T13:41:07Z

spec/integration_spec.rb

-    expect(results['hits'].length).to eq(0)
-  end
-
-  it 'sanitizes attributes' do


Moved to options_spec.rb since it tests whether or not the :sanitize option is behaving correctly.

ellnix · 2024-03-01T13:41:52Z

spec/integration_spec.rb

-      expect(b['hits'][0]['author']).to eq('alert(1)')
-      expect(b['hits'][0]['_formatted']['name']).to eq('"> <em>hack</em>0r')
-    rescue StandardError
-      # rails 4.2's sanitizer


Kept only the last sanitization format.

ellnix · 2024-03-01T13:43:46Z

spec/integration_spec.rb

-    expect(index.uid).to eq("#{safe_index_uid('Book')}_#{Rails.env}")
-  end
-
-  it 'searches with one typo min size' do


Moved to settings_spec.rb since it tests the typo_tolerance setting

ellnix · 2024-03-01T13:44:02Z

spec/integration_spec.rb

-    expect(results.size).to eq(1)
-  end
-
-  describe '#ms_entries' do


Moved to instance_methods_spec.rb

ellnix · 2024-03-01T13:44:18Z

spec/integration_spec.rb

-    end
-  end
-
-  it 'returns facets using max values per facet' do


Moved to settings_spec.rb

ellnix · 2024-03-01T13:44:57Z

spec/integration_spec.rb

-    expect(results.facets_distribution['genre'].size).to eq(3)
-  end
-
-  it 'does not error on facet_search' do


Moved to new safe_index_spec.rb since this is a test on whether SafeIndex forwards method arguments correctly

ellnix · 2024-03-01T13:46:28Z

spec/support/models/book.rb

@@ -32,3 +32,11 @@ def public?
    released && !premium
  end
 end
+
+module TestUtil
+  def self.reset_books!


Moved the index and database resetting logic to a method. Since usage of Book is scattered we can no longer use a before hook for this.

ellnix · 2024-03-01T14:09:26Z

spec/integration_spec.rb

-    Color.delete_all
-  end
-
-  it 'auto indexes' do


Already tested in options_spec.rb under :auto_index

ellnix · 2024-03-01T14:09:57Z

spec/integration_spec.rb

-  end
-
-
-  it 'is able to temporarily disable auto-indexing' do


Moved to model_methods_spec.rb

ellnix · 2024-03-01T14:10:12Z

spec/integration_spec.rb

-    expect(Color.search('blue').size).to eq(1)
-  end
-
-  it 'updates the index if the attribute changed' do


Moved to record_is_updated_spec.rb

ellnix · 2024-03-01T14:10:25Z

spec/integration_spec.rb

-    expect(Color.search('pink').size).to eq(1)
-  end
-
-  it 'uses the specified scope' do


Moved to model_methods_spec.rb

ellnix · 2024-03-01T14:10:42Z

spec/integration_spec.rb

-    expect(Color.search('').size).to eq(1)
-  end
-
-  it 'indexes an array of documents' do


Moved to model_methods_spec.rb

ellnix · 2024-03-01T14:11:06Z

spec/integration_spec.rb

-    expect(json['hits'].count).to eq(Color.raw_search('')['hits'].count)
-  end
-
-  it 'does not index non-saved document' do


Moved to instance_method_specs.rb

ellnix · 2024-03-01T14:11:36Z

spec/support/models/color.rb

@@ -35,3 +35,10 @@ def will_save_change_to_short_name?
    false
  end
 end
+
+module TestUtil
+  def self.reset_colors!


Same rationale as reset_books!

ellnix · 2024-03-01T14:39:03Z

spec/integration_spec.rb

@@ -631,41 +631,6 @@
  end
 end

-describe 'Animals' do
-  it 'returns only the requested type' do


Moved to options_spec.rb under (shared index)

ellnix · 2024-03-01T14:39:12Z

spec/integration_spec.rb

-    expect(Dog.search('Toby').first.name).to eq('Toby the Dog')
-  end
-
-  it 'shares a single index' do


Moved to options_spec.rb

ellnix · 2024-03-01T14:39:26Z

spec/integration_spec.rb

-    expect(cat_index).to eq(dog_index)
-  end
-
-  describe '#ms_entries' do


Moved to instance_methods_spec.rb

ellnix · 2024-03-02T15:35:48Z

spec/integration_spec.rb

-
-  after(:all) { MeiliSearch::Rails.configuration[:per_environment] = true }
-
-  it 'adds custom complex attribute' do


Moved to settings_spec.rb

ellnix · 2024-03-02T15:36:15Z

spec/integration_spec.rb

-    expect(result['hits'][0]['full_name']).to eq('Jane Doe')
-  end
-
-  it 'has as uid the custom name specified' do


Moved to options_spec.rb

ellnix · 2024-03-02T15:36:31Z

spec/integration_spec.rb

-    expect(People.index.uid).to eq(safe_index_uid('MyCustomPeople'))
-  end
-
-  it 'has the chosen field as custom primary key' do


Moved to options_spec.rb

ellnix · 2024-03-02T15:36:48Z

spec/integration_spec.rb

-    expect(index.primary_key).to eq('card_number')
-  end
-
-  it 'does not call the API if there has been no attribute change' do


Moved to record_is_updated_spec.rb

ellnix · 2024-03-02T15:37:18Z

spec/integration_spec.rb

-    end.not_to change(People.index.tasks['results'], :size)
-  end
-
-  it 'does not auto-remove' do


Moved to options_spec.rb

ellnix · 2024-03-02T15:37:33Z

spec/integration_spec.rb

-    expect(result['hits'].size).to eq(1)
-  end
-
-  it 'is able to remove manually' do


Moved to instance_methods_spec.rb

ellnix · 2024-03-02T15:37:47Z

spec/integration_spec.rb

-    expect(result['hits'].size).to eq(0)
-  end
-
-  it 'clears index manually' do


Moved to model_methods_spec.rb

Should look into adding pagy specs in the future, along with meilisearch#362

This is mainly focused around removing interdependencies between tests and ensuring that there are no race conditions.

ellnix · 2024-05-25T17:22:23Z

spec/system/tech_shop_spec.rb

Forgot to mention in the commit, but is me adding another layer of tests (system tests).

We would have

Unit tests

Integration tests (checking that systems fit together given simple scenarios)

System tests (do common tasks with a setup of realistic data)

Acceptance tests (reserved for full-stack tests, i. e. running a rails app and testing it with selenium verify with 100% certainty that a small subset of setups that we lay out work. Could also be helpful in letting us see what needs to be updated on a user's end when we make changes)

ellnix · 2024-05-25T17:25:40Z

spec/system/tech_shop_spec.rb

 require 'support/models/product'

 describe 'Tech shop' do
  before(:all) do
-    Product.clear_index!(true)


Funnily enough, this never worked! Product has indexing disabled and in cases where indexing is disabled clear_index does nothing.

However, this did not present an issue since this one describe block was the only thing using product. When this runs after the recently-added multi search specs where I also used product, many of the expectations fail 🤣

I found out the hard way as you can imagine.

ellnix · 2024-05-25T17:26:40Z

spec/system/tech_shop_spec.rb

@@ -37,150 +39,142 @@
    # Subproducts
    @camera = Camera.create!(name: 'canon eos rebel t3', href: 'canon')

-    100.times { Product.create!(name: 'crapoola', href: 'crappy', tags: ['crappy']) }


This alone removed over half a second from the test runtime for me. These 100 "crapoola"s are actually used in the specs.

ellnix · 2024-05-25T17:27:22Z

spec/system/tech_shop_spec.rb

@@ -37,150 +39,142 @@
    # Subproducts
    @camera = Camera.create!(name: 'canon eos rebel t3', href: 'canon')

-    100.times { Product.create!(name: 'crapoola', href: 'crappy', tags: ['crappy']) }
-
-    @products_in_database = Product.all


This should probably save on memory, but the biggest difference it'll probably make is removing a line of code that we never needed.

ellnix · 2024-05-25T17:28:18Z

spec/system/tech_shop_spec.rb

+
+      products_after_clear = Product.raw_search('')['hits']
+      expect(products_after_clear).to be_empty
+      Product.reindex!(MeiliSearch::Rails::IndexSettings::DEFAULT_BATCH_SIZE, true)


This syntax is still clunky but I'm saving it for when I refactor the actual lib, not the test suite.

ellnix · 2024-05-25T17:31:21Z

spec/system/tech_shop_spec.rb

      ipad.destroy
-      results = Product.search('ipad')
-      expect(results.size).to eq(0)


Fun fact: this actually tested nothing.

As one of the later tests asserts, if there is a document in the ms index that cannot be found in AR, it won't be returned by search (although it is returned by raw_search).

This is why this never caused any errors despite the obvious-looking race condition between this assertion and the above ipad.destroy line.

ellnix · 2024-05-25T17:32:10Z

spec/system/tech_shop_spec.rb

        Product.index.add_documents!(@palmpre.attributes.merge(id: -1))
-        expect(JSON.parse(Product.search('pal').to_json).size).to eq(2)


I do not understand why this was being converted to json and back. If there's some edge case or special significance to this please let me know.

It looks to me like this method simply tested that we define a `#search` method on models, but if we didn't define that method most of the test suite would fail. If these was a different purpose to this test I don't see it.

- Searchable attribute warning only happened on the top level block, with this fix it will work on additional indexes (added with `add_index`) - This involved moving the warning method to IndexSettings which has the side effect of having no access to the model name, however the attribute name alone *should* be enough for a user to figure out the error - The searchable attribute spec created a new class which due to ActiveModel naming and our interdependence on it cannot conveniently be isolated from other tests

The specs should now be more resilient by using regular expressions instead of index names.

New test has been added while PR meilisearch#350 was being finished.

- Specs are all moved to their own files! - The remaining tests were unit tests on the `meilisearch_settings_changed?` method which should be tested but not this way. Postponing until the logic has been refactored out of the MeiliSearch::Rails module. - Do not require every ActiveRecord table in every test - Some require statements that were missing were added in this commit

This should mean less noise in the test, and if we follow this example going foward when adding Sequel and Mongoid models the test output should be relevant only to the tests being run.

ellnix · 2024-05-27T18:35:44Z

@brunoocasali I tried to keep the commits as small as possible, and annotated a lot of the earlier ones. I would recommend going through this commit by commit rather than through Files changed.

brunoocasali

LGTM!

bors merge

ellnix added 9 commits February 29, 2024 14:43

Remove redundant Ebook model in specs

fc7bbac

Remove test of ruby's Marshal

c5990b3

This test seems to just use Marshal.dump on an object and then Marshal.load it later, which would pass for any ruby object.

Remove unused UniqUser class from specs

4c62567

Remove unused NullableId class from specs

0994f36

Remove flawed MongoDocument specs

355250e

It does not seem like Mongoid::Document has bang methods, so this is spec is not reproducing a likely naming conflict. In either case, the plan is to test with real MongoDB models so this test would be removed eventually.

Remove unnecessary Color tests

093bfbc

'is_synchronous' tests a private method and only tests that the configuration was applied 'has maxValuesPerFacet set' tests meilisearch-ruby's Index class

Move Post specs

26e607c

Move Color search specs

3fda1a7

Move Book specs from integration_spec.rb

8504a7a

ellnix commented Mar 1, 2024

View reviewed changes

Move Color specs from integration_spec.rb

b0cc102

ellnix commented Mar 1, 2024

View reviewed changes

Move Cat and Doc specs from integration_spec.rb

b9a278c

ellnix commented Mar 1, 2024

View reviewed changes

ellnix added 2 commits March 1, 2024 16:04

Move People specs from integration_spec.rb

4663f91

Move Disabled specs from integration_spec.rb

5c7a6e4

ellnix commented Mar 2, 2024

View reviewed changes

ellnix added 4 commits May 25, 2024 16:14

Move Movie, Restaurant, and pagination specs

55bf813

Should look into adding pagy specs in the future, along with meilisearch#362

require --spec-helper in .rspec

91ac1af

Move 'an imaginary store' tests

4cf7ad7

Refactor tests

a437c8e

This is mainly focused around removing interdependencies between tests and ensuring that there are no race conditions.

ellnix commented May 25, 2024

View reviewed changes

ellnix added 17 commits May 26, 2024 09:31

Move SerializedDocument and EncodedString tests

50ec330

Move NamespacedDocument specs

702888b

Remove redundant #search method test

862262b

It looks to me like this method simply tested that we define a `#search` method on models, but if we didn't define that method most of the test suite would fail. If these was a different purpose to this test I don't see it.

Move deactivation specs

5deaef9

Move :raise_on_failure specs

208c9cb

Fix a few more race conditions in tests

88bed6f

Move EnqueuedDocument-type tests

596a7b4

Move Song specs

b328a69

Move NestedItem tests

4f1663b

Move misconfiguration test

dcc4c08

Fix regression in #ms_entries specs

121b989

The specs should now be more resilient by using regular expressions instead of index names.

Merge test refactoring into main

84d355b

New test has been added while PR meilisearch#350 was being finished.

Refactor proximity_precision test

eefe6be

Hide ActiveRecord's create_table messages

300b778

This should mean less noise in the test, and if we follow this example going foward when adding Sequel and Mongoid models the test output should be relevant only to the tests being run.

Make rubocop happy

f1b4ec3

ellnix mentioned this pull request May 27, 2024

Remove redundant tests #345

Closed

Merge main again due to rubocop and integration_spec

1d70659

ellnix marked this pull request as ready for review May 27, 2024 18:30

Fix will_paginate test race condition

cf8a17f

ellnix requested a review from brunoocasali May 27, 2024 18:34

brunoocasali approved these changes Jul 24, 2024

View reviewed changes

Merge branch 'main' into refactor-integration-spec

487c780

brunoocasali merged commit 5b57c6b into meilisearch:main Aug 7, 2024
10 of 11 checks passed

curquiza added the maintenance Anything related to maintenance (CI, tests, refactoring...) label Sep 24, 2024


		after(:all) { MeiliSearch::Rails.configuration[:per_environment] = true }

		it 'adds custom complex attribute' do

		Product.index.add_documents!(@palmpre.attributes.merge(id: -1))
		expect(JSON.parse(Product.search('pal').to_json).size).to eq(2)

Split spec/integration_spec.rb #350

Split spec/integration_spec.rb #350

Conversation

ellnix commented Feb 29, 2024 • edited Loading

codecov bot commented Feb 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellnix May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellnix commented May 27, 2024

brunoocasali left a comment

Choose a reason for hiding this comment

Split `spec/integration_spec.rb` #350

Split `spec/integration_spec.rb` #350

ellnix commented Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

ellnix May 25, 2024 •

edited

Loading