Tech 4739 add unit test framework support #9
Conversation
…ted/unsupported Rails versions
| @@ -1,4 +1,17 @@ | |||
| #!/usr/bin/env rake | |||
| # frozen_string_literal: true | |||
| # Add your own tasks in files placed in lib/tasks ending in .rake, | |||
There was a problem hiding this comment.
I think the above 2 deleted lines should stay up there.
Oh, but now I'm guessing this all came from the Rails generator? It's not worth changing then.
| module Enum | ||
|
|
||
| class << self | ||
| def current_mysql_column_adapter |
There was a problem hiding this comment.
Was it actually helpful for testing having this in an eigenclass method like this? I was fine with the earlier version that did this part inline to initialize ActiveRecordColumnWithEnums.
There was a problem hiding this comment.
Yeah, I liked the way the tests were written with these eigenclass methods.
expect { ActiveRecord::Mysql::Enum.current_mysql_column_adapter }.to raise_error(RuntimeError, "could not find MySQL::Column or equivalent connection adapter")
| when Symbol | ||
| value | ||
| when String | ||
| value.to_sym if value.present? |
There was a problem hiding this comment.
Odd that other values just fall out as nil. I think it would be a bug if that happened, except perhaps for nil itself. I wonder if this should have
case nil
nil
else
raise ArgumentError, "expected Symbol, String, or nil (got #{value.inspect})"
There was a problem hiding this comment.
Hmm, I hadn't thought about it before - I was just getting rid of the redundant else nil case. Do you know offhand if the non-Enum column types raise rather than falling out as nil? I'll look it up shortly, but I think this should be consistent.
There was a problem hiding this comment.
I was only suggesting case nil so you could else raise on surprise values. That's the important part: this code would seem out of control if, say, you passed in a Hash and it returned nil.
| if defined? ActiveRecord::ConnectionAdapters::Mysql2Adapter | ||
| ActiveRecord::ConnectionAdapters::Mysql2Adapter | ||
| else | ||
| raise "Could not find MySQL connection adapter" |
There was a problem hiding this comment.
Because raise if flow-of-control, you don't need it in an else. You can always flip it:
defined?(ActiveRecord::ConnectionAdapters::Mysql2Adapter) or raise "Could not find MySQL connection adapter"
ActiveRecord::ConnectionAdapters::Mysql2Adapter
I'm not sure this needs to be in a method either. It seems like it would be fine below. Especially since it doesn't really have any 'current' logic like the name implies.
There was a problem hiding this comment.
I'll drop the "current" bit and rename it as just mysql_adapter and I'll make the same change for the current_mysql_column_adapter above. I'll also flip the guard clause to the top here, good point.
Same as with the column adapter module, I like having this method because it makes testing nice so I'd prefer to leave it.
spec/unit/mysql_adapter_spec.rb
Outdated
| context "#type_to_sql" do | ||
| let(:expected_enum_sql_type) { "enum('a','b','c')" } | ||
|
|
||
| if Rails::VERSION::MAJOR < 5 |
There was a problem hiding this comment.
Can this be more DRY? These tests seem about the same. Could you encapsulate the differences in a function or two that did the Rails::VERSION::MAJOR < 5 test themselves?
spec/unit/schema_definitions_spec.rb
Outdated
| expect(subject.default).to eq(column_options[:default]) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Looks like your editor isn't set to put trailing newlines in the files?
1.0.0
Added
Changed
Removed