Skip to content
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

Add tests of how boolean columns worked in 1.6.3 #507

Conversation

jaredbeck
Copy link
Contributor

The string '1', in 1.6.3, was translated into boolean true.

These tests pass in 1.6.3 and fail after 9f4e48e.

CC: #503

@jaredbeck
Copy link
Contributor Author

Hmm looks like mysql uses a 1 for boolean true, while others use a char 't'. So, the mysql tests are failing. Oh well, you get the idea. :)

@jaredbeck
Copy link
Contributor Author

Maybe the test could switch on ENV['DB'], or ActiveRecord::Base.connection.adapter_name?

Or we could drop the right-hand side of the SQL comparison in our test assertion. Just testing the presence of the column would be an improvement over master.

@jaredbeck jaredbeck force-pushed the searching_boolean_columns branch from cbd65b4 to 0534d09 Compare February 2, 2015 16:50
The string '1', in 1.6.3, was translated into boolean true.

These tests pass in 1.6.3 and fail after 9f4e48e.
@jaredbeck jaredbeck force-pushed the searching_boolean_columns branch from 0534d09 to 4219fdf Compare February 2, 2015 17:18
@avit
Copy link
Contributor

avit commented Feb 2, 2015

Suggest: sql_true = Person.connection.quote(true)

@jonatack
Copy link
Contributor

jonatack commented Feb 2, 2015

OK, just saw this, thanks 👍 My initial thought is to prefer adding these tests in the predicate_spec.rb suite and test for all the values. Let's see what Travis-ci says about it.

jonatack referenced this pull request Feb 2, 2015
These tests are meant to prevent changing the TRUE_VALUES and
FALSE_VALUES sets in RANSACK::CONSTANTS without a very good reason.

Previously, no test was alerting us against merging changes like #503.
@jaredbeck
Copy link
Contributor Author

Suggest: sql_true = Person.connection.quote(true)

Nice. Thanks! Is there a quoting method for table.column?

@jonatack
Copy link
Contributor

jonatack commented Feb 2, 2015

Yes:

def expected_boolean_eq_query(boolean_value)
  field = "#{quote_table_name("people")}.#{quote_column_name("awesome")}"
  condition = ActiveRecord::Base.connection.quote(boolean_value)
  /#{field} = #{condition}/
end

@jaredbeck
Copy link
Contributor Author

Cool, I'll give quote_column_name a try. I was unsure, because I didn't see an implementation of it in e.g. https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb

@jonatack jonatack closed this Feb 2, 2015
@avit
Copy link
Contributor

avit commented Feb 2, 2015

Thanks guys! :)

@jonatack
Copy link
Contributor

jonatack commented Feb 2, 2015

Thanks to both of you for the heads up! 💚

@jaredbeck
Copy link
Contributor Author

My initial thought is to prefer adding these tests in the predicate_spec.rb suite and test for all the values.

Makes sense, especially the "test for all the values" part. My tests here only cover char '0' and '1'. Thanks!

@jonatack
Copy link
Contributor

jonatack commented Feb 2, 2015

@avit thanks for the suggestion above. In the last commit 5009eea, following your suggestion with:

condition = ActiveRecord::Base.connection.quote(boolean_value)

would be an improvement over

condition = ActiveRecord::Base.connection.send("quoted_#{boolean_value}")

If you'd like to change it.

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