Skip to content

broken tests for using each_with_hit and search result order #102

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

Closed
wants to merge 2 commits into from

Conversation

dwkoogt
Copy link
Contributor

@dwkoogt dwkoogt commented May 5, 2014

broken tests for using each_with_hit when the returned records should not be the natural order.

this is for issue #99

searching for 'code' returns 'Coding' first and 'Test Coding' second from elasticsearch.
it compares ids of db records to response result.

note: calling to_a within the each_with_hit method changes the _id behavior on the mongoid record. it returned string id originally but after to_a is called, it returns ObjectId instead.

… not be the natural order.

searching for 'code' returns 'Coding' first and 'Test Coding' second from elasticsearch.
it compares ids of db records to response result.
note: calling to_a within the each_with_hit method changes the _id behavior on the mongoid record. it returned string id originally but after to_a is called, it returns ObjectId instead.
@karmi
Copy link
Contributor

karmi commented May 6, 2014

@dwkoogt I've run bundle exec ruby -I lib:test test/integration/active_record_basic_test.rb on your branch now, and not getting any errors -- what kind of error do you get?

@dwkoogt
Copy link
Contributor Author

dwkoogt commented May 6, 2014

active_record_basic_test.rb is a passing test just to illustrate the problem with mongoid. You need to run mongoid_basic_test.rb

@karmi
Copy link
Contributor

karmi commented May 6, 2014

So the problem is solely with the Mongoid adapter?

@karmi
Copy link
Contributor

karmi commented May 6, 2014

Right, so I can replicate the problem with Mongoid, the fix seems to be in calling to_a in the each_with and map_with methods.

Can you please sign the Contributor License Agreement so I can merge your test in?

@@ -81,6 +81,16 @@ class ::Article < ActiveRecord::Base
end
end

should "zip results from records" do
Copy link
Contributor

Choose a reason for hiding this comment

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

We should name this better, something like should "preserve the search results order for records" do

@dwkoogt
Copy link
Contributor Author

dwkoogt commented May 6, 2014

Contributor License Agreement has been signed.

karmi added a commit that referenced this pull request May 7, 2014
…rch::Model::Response::Records` call `to_a`

The `zip` method in `records.rb` didn't call `to_a` on the collection,
thus the records were not returned in the same order as Elasticsearch's results.

Related #99
Related #102
@karmi karmi closed this in 104f91f May 7, 2014
@karmi
Copy link
Contributor

karmi commented May 7, 2014

Thanks for the changes!, added the ``to_a` fix and merged in your commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants