Skip to content

Conversation

@yahonda
Copy link
Member

@yahonda yahonda commented Sep 13, 2017

Summary

This pull request addresses random test_or_with_bind_params failures reported at https://travis-ci.org/rails/rails/jobs/274370258

Failure:
ActiveRecord::OrTest#test_or_with_bind_params [/home/travis/build/rails/rails/activerecord/test/cases/relation/or_test.rb:34]:
--- expected
+++ actual
@@ -1 +1 @@
-[#<Post id: 1, author_id: 1, title: "Welcome to the weblog", body: "Such a lovely day", type: "Post", comments_count: 2, taggings_with_delete_all_count: 0, taggings_with_destroy_count: 0, tags_count: 1, tags_with_destroy_count: 0, tags_with_nullify_count: 0>, #<SpecialPost id: 2, author_id: 1, title: "So I was thinking", body: "Like I hopefully always am", type: "SpecialPost", comments_count: 1, taggings_with_delete_all_count: 0, taggings_with_destroy_count: 0, tags_count: 1, tags_with_destroy_count: 0, tags_with_nullify_count: 0>]
+[#<SpecialPost id: 2, author_id: 1, title: "So I was thinking", body: "Like I hopefully always am", type: "SpecialPost", comments_count: 1, taggings_with_delete_all_count: 0, taggings_with_destroy_count: 0, tags_count: 1, tags_with_destroy_count: 0, tags_with_nullify_count: 0>, #<Post id: 1, author_id: 1, title: "Welcome to the weblog", body: "Such a lovely day", type: "Post", comments_count: 2, taggings_with_delete_all_count: 0, taggings_with_destroy_count: 0, tags_count: 1, tags_with_destroy_count: 0, tags_with_nullify_count: 0>]
bin/rails test home/travis/build/rails/rails/activerecord/test/cases/relation/or_test.rb:33
  • Post.find([1, 2]) generates this query below:
SELECT "posts".* FROM "posts" WHERE "posts"."id" IN ($1, $2)  [["id", 1], ["id", 2]]
  • Post.where(id: 1).or(Post.where(id: 2)).to_a generates this query below:
SELECT "posts".* FROM "posts" WHERE ("posts"."id" = $1 OR "posts"."id" = $2)  [["id", 1], ["id", 2]]

Most of the time these two queries return the same result but the order of records are not guaranteed
from SQL point of view then added sort before comparing them.

Also changed id values to 10 and 11 to make it easy to sort an array to avoid ArgumentError: comparison of SpecialPost with Post failed.

Other Information

This failure is easy to reproduce by using reverse_fixtures branch which reverses the fixture

$ git clone -b reverse_fixtures https://github.com/yahonda/rails.git
$ cd rails/activerecord
$ bundle
$ ARCONN=postgresql bin/test test/cases/relation/or_test.rb:33

@rails-bot
Copy link

r? @kamipo

(@rails-bot has picked a reviewer for you, use r? to override)

Copy link
Member

Choose a reason for hiding this comment

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

How about adding .sort_by(&:id) simply?

--- a/activerecord/test/cases/relation/or_test.rb
+++ b/activerecord/test/cases/relation/or_test.rb
@@ -31,7 +31,7 @@ def test_or_with_null_right
     end
 
     def test_or_with_bind_params
-      assert_equal Post.find([1, 2]), Post.where(id: 1).or(Post.where(id: 2)).to_a
+      assert_equal Post.find([1, 2]).sort_by(&:id), Post.where(id: 1).or(Post.where(id: 2)).sort_by(&:id)
     end
 
     def test_or_with_null_both

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. Much better one since no need to change id values.

Reported at https://travis-ci.org/rails/rails/jobs/274370258

- `Post.find([1, 2])` generates this query below:
```sql
SELECT "posts".* FROM "posts" WHERE "posts"."id" IN ($1, $2)  [["id", 1], ["id", 2]]
```

- `Post.where(id: 1).or(Post.where(id: 2)).to_a` generates this query below:
```sql
SELECT "posts".* FROM "posts" WHERE ("posts"."id" = $1 OR "posts"."id" = $2)  [["id", 1], ["id", 2]]
```

Most of the time these two queries return the same result but the order of records are not guaranteed
from SQL point of view then added `sort` before comparing them.
@yahonda yahonda force-pushed the address_test_or_with_bind_params_failure branch from 409d214 to aaed081 Compare September 13, 2017 23:01
@kamipo kamipo merged commit 5a4cd4f into rails:master Sep 13, 2017
@yahonda yahonda deleted the address_test_or_with_bind_params_failure branch September 27, 2017 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants