Skip to content

Conversation

@fractaledmind
Copy link
Owner

@riyengar8 @barendt

This is a quite the branch. It uses the gemika gem to setup running the test suite (which is already trying to be as thorough and expansive as reasonable for the business logic) against a matrix of rails versions, ruby versions, and database engines. Once I got the test suite set up, I started running different scenarios and trying to fix things. MySQL has been quite the pain in my ass, suffice it to say.

The core code for the business logic isn't pretty or finalized. I want to optimize for getting the matrix of tests passing with as little indirection as seems reasonable and then take a step back at that point to consider how best to slice up the objects and functions.

If you want to run tests for a particular scenario, you will need to know about a couple of things:

  • set the Ruby version you want for the scenario either via the .ruby-version file or using the RBENV_VERSION env variable
  • set the Rails version and database engine you want to use by specifying which Gemfile you want from the options in the /gemfiles directory
  • there are a number of options you can set for the test suite:
    • LOGICALLY_EXHAUSTIVE_REQUEST_SPECS=true will run, well, the full logically exhaustive set of tests; whereas, by default, the test suite just runs a random sampling. This is needed if you want to rerun a particular test, as without this flag you will get a different random test at that location
    • INSPECT_FAILURE=true will output the data in the instance variables used in the failed test

For example, this is how I have been testing Rails 4.2.x with Ruby 2.6.5 and MySQL:

RBENV_VERSION=2.6.5 BUNDLE_GEMFILE=gemfiles/Gemfile\[rails-4.2.x\]\[mysql2-0.4.x\] LOGICALLY_EXHAUSTIVE_REQUEST_SPECS=true INSPECT_FAILURE=true bundle exec rspec spec

# Conflicts:
#	Gemfile.lock
#	Rakefile
# Conflicts:
#	.ruby-version
#	.travis.yml
#	Gemfile.lock
def test_sort_nested_object_bool_then_int_in_ascending_with_nils_large_then_ascending_with_nils_large_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :large },
{ direction: :ascending, accessor: 'assoc.int', nils: :large })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_nested_object_bool_then_int_in_ascending_with_nils_small_then_ascending_with_nils_large_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :small },
{ direction: :ascending, accessor: 'assoc.int', nils: :large })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_nested_object_bool_then_int_in_ascending_with_nils_large_then_ascending_with_nils_small_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :large },
{ direction: :ascending, accessor: 'assoc.int', nils: :small })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_nested_object_bool_then_int_in_ascending_with_nils_small_then_ascending_with_nils_small_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :small },
{ direction: :ascending, accessor: 'assoc.int', nils: :small })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

boolean_nil_tuple_array = boolean_array.product(nil_array)
nil_integer_tuple_array = nil_array.product(integer_array)

@array_of_tuples_to_array_of_objs_transformer = ->(array) { array.map { |bool, int| obj.new(bool, assoc.new(int)) } }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [121/120]

def test_sort_nested_object_bool_then_int_in_descending_then_ascending_order
assert_equal(
Sorter.new({ direction: :descending, accessor: :bool },
{ direction: :ascending, accessor: 'assoc.int' })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_nested_object_bool_then_int_in_ascending_then_descending_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool },
{ direction: :descending, accessor: 'assoc.int' })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_nested_object_bool_then_int_in_ascending_then_ascending_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool },
{ direction: :ascending, accessor: 'assoc.int' })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

boolean_array = [true, false]
integer_array = (1..2).to_a

@array_of_tuples_to_array_of_objs_transformer = ->(array) { array.map { |bool, int| obj.new(bool, assoc.new(int)) } }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [121/120]

def test_sort_object_bool_then_int_in_ascending_with_nils_large_then_ascending_with_nils_large_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :large },
{ direction: :ascending, accessor: :int, nils: :large })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

require 'gemika'
require 'combustion'

class Combustion::Database::Reset
Copy link

Choose a reason for hiding this comment

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

IrresponsibleModule: Combustion::Database::Reset has no descriptive comment. More info.

end

def value_for(object:, path:)
def attr_value_for(object:, path:)
Copy link

Choose a reason for hiding this comment

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

UtilityFunction: PathHelpers#attr_value_for doesn't depend on instance state (maybe move it to another class?). More info.

# In order to use equality matchers for :float fields in MySQL, we need to cast to :decimal
# https://dev.mysql.com/doc/refman/8.0/en/problems-with-float.html
Arel::Nodes::NamedFunction.new('CAST', [arel_column.as('DECIMAL(8,2)')])
elsif arel_type_is_string_or_text_or_binary && arel_operator_is_between_type && arel_column_collation_is_not_comparable
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [145/120]

# NOTE: this is can be quite inefficient, as it forces the DB engine to perform that cast on all rows.
# https://www.ryadel.com/en/like-operator-equivalent-integer-numeric-columns-sql-t-sql-database/
Arel::Nodes::NamedFunction.new('CAST', [arel_column.as('CHAR')])
elsif adapter_is_mysql && arel_type_is_float && (query_value_is_numeric || query_value_is_collection_of_numerics || query_value_is_range_of_numerics)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [175/120]


@query_column = if arel_type_integer_or_float && arel_operator_is_match_type
# In order to use LIKE, we must CAST the column as a CHAR column.
# NOTE: this is can be quite inefficient, as it forces the DB engine to perform that cast on all rows.
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [130/120]

if v.respond_to?(:map)
v.map { |vv| sprintf("%0.#{arel_column_scale}f", vv) }
else
sprintf("%0.#{arel_column_scale}f", v)
Copy link

Choose a reason for hiding this comment

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

Style/FormatString: Favor format over sprintf.

if value.respond_to?(:map)
value.map do |v|
if v.respond_to?(:map)
v.map { |vv| sprintf("%0.#{arel_column_scale}f", vv) }
Copy link

Choose a reason for hiding this comment

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

Style/FormatString: Favor format over sprintf.

adapter_is_mysql = adapter_type.presence_in(%i[mysql mysql2])
arel_column = attribute_model.column_for_attribute(attribute)
arel_column_scale = arel_column&.scale
value = begin
Copy link

Choose a reason for hiding this comment

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

Style/MultilineIfModifier: Favor a normal if-statement over a modifier clause in a multiline statement.

end

private
def query_value
Copy link

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for query_value is too high. [43.47/15]
Metrics/CyclomaticComplexity: Cyclomatic complexity for query_value is too high. [13/6]
Metrics/PerceivedComplexity: Perceived complexity for query_value is too high. [17/7]


included do
ApplicationRecord::DB_FIELD_TYPES.each do |field|
scope "#{field}_scope_method", (lambda do |v|
Copy link

Choose a reason for hiding this comment

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

UncommunicativeVariableName: Scopes has the variable name 'v'. More info.

def test_sort_object_bool_then_int_in_ascending_with_nils_small_then_ascending_with_nils_large_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :small },
{ direction: :ascending, accessor: :int, nils: :large })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_object_bool_then_int_in_ascending_with_nils_large_then_ascending_with_nils_small_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :large },
{ direction: :ascending, accessor: :int, nils: :small })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_object_bool_then_int_in_ascending_with_nils_small_then_ascending_with_nils_small_order
assert_equal(
Sorter.new({ direction: :ascending, accessor: :bool, nils: :small },
{ direction: :ascending, accessor: :int, nils: :small })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_object_int_then_bool_in_descending_then_descending_order
assert_equal(
Sorter.new({ direction: :descending, accessor: :int },
{ direction: :descending, accessor: :bool })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def test_sort_object_int_then_bool_in_descending_then_ascending_order
assert_equal(
Sorter.new({ direction: :descending, accessor: :int },
{ direction: :ascending, accessor: :bool })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

def segment(string) # https://stackoverflow.com/a/15170063/2884386
string # "a12b34c"
.scan(/\d+|\D/) # ["a", "12", "b", "34", "c"]
.map { |a| a =~ /\d+/ ? a.to_i : a } # ["a", 12, "b", 34, "c"]
Copy link

Choose a reason for hiding this comment

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

Performance/RegexpMatch: Use match? instead of =~ when MatchData is not used.

.join # "aaaasss"
end

def segment(string) # https://stackoverflow.com/a/15170063/2884386
Copy link

Choose a reason for hiding this comment

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

Style/CommentedKeyword: Do not place comments on the same line as the def keyword.

string # "Äaáäßs"
.unicode_normalize(:nfd) # "Äaáäßs"
.downcase(:fold) # "äaáässs"
.chars # ["a", "̈", "a", "a", "́", "a", "̈", "s", "s", "s"]
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.

def normalize(string) # https://github.com/grosser/sort_alphabetical
string # "Äaáäßs"
.unicode_normalize(:nfd) # "Äaáäßs"
.downcase(:fold) # "äaáässs"
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.


def normalize(string) # https://github.com/grosser/sort_alphabetical
string # "Äaáäßs"
.unicode_normalize(:nfd) # "Äaáäßs"
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.

end

def normalize(string) # https://github.com/grosser/sort_alphabetical
string # "Äaáäßs"
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.

.to_r # (980130990351/10000000000)
end

def normalize(string) # https://github.com/grosser/sort_alphabetical
Copy link

Choose a reason for hiding this comment

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

Style/CommentedKeyword: Do not place comments on the same line as the def keyword.

.to_r # (24266512014313/250000000000)
end

def time_to_numeric_value(time) # https://stackoverflow.com/a/30604935/2884386
Copy link

Choose a reason for hiding this comment

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

Style/CommentedKeyword: Do not place comments on the same line as the def keyword.


def string_to_numeric_value(string)
string # "aB09ü""
.chars # ["a", "B", "0", "9", "ü"]
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.

end

def string_to_numeric_value(string)
string # "aB09ü""
Copy link

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.

Operators.get(instruction_operator)
def arel_column_to_sql(arel_column)
# the ARel column can already be rendered as SQL (because it is wrapped in a LOWER function, for example)
if arel_column.respond_to?(:to_sql)
Copy link

Choose a reason for hiding this comment

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

ManualDispatch: ActiveSet::Filtering::ActiveRecord::SetInstruction#arel_column_to_sql manually dispatches method call. More info.

end

Operators.get(instruction_operator)
def arel_column_to_sql(arel_column)
Copy link

Choose a reason for hiding this comment

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

UtilityFunction: ActiveSet::Filtering::ActiveRecord::SetInstruction#arel_column_to_sql doesn't depend on instance state (maybe move it to another class?). More info.

unwrap_sql_str(sql_value)
end

def unwrap_sql_str(sql_str)
Copy link

Choose a reason for hiding this comment

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

UtilityFunction: ActiveSet::Filtering::ActiveRecord::SetInstruction#unwrap_sql_str doesn't depend on instance state (maybe move it to another class?). More info.

end

def to_sql_str(value)
return value.map { |a| to_sql_str(a) } if value.respond_to?(:map)
Copy link

Choose a reason for hiding this comment

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

ManualDispatch: ActiveSet::Filtering::ActiveRecord::SetInstruction#to_sql_str manually dispatches method call. More info.
UncommunicativeVariableName: ActiveSet::Filtering::ActiveRecord::SetInstruction#to_sql_str has the variable name 'a'. More info.

.to_sym
end

def to_sql_str(value)
Copy link

Choose a reason for hiding this comment

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

TooManyStatements: ActiveSet::Filtering::ActiveRecord::SetInstruction#to_sql_str has approx 6 statements. More info.

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.

2 participants