diff --git a/NEWS.md b/NEWS.md index cf7cd544..f02fac5c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -48,6 +48,15 @@ is now: * *PRs: [#989], [#964], [#917]* * *Original issue: [#867]* +### Features + +* Add `required` and `optional` qualifiers to `belong_to` and `have_one` + matchers. (When using the `belong_to` matcher under Rails 5+, `required` is + assumed unless overridden.) + + * *Original PR: [#956]* + * *Original issues: [#870], [#861]* + [a6d09aa]: https://github.com/thoughtbot/shoulda-matchers/commit/a6d09aa5de0d546367e7b3d7177dfde6c66f7f05 [#943]: https://github.com/thoughtbot/shoulda-matchers/pulls/943 [#933]: https://github.com/thoughtbot/shoulda-matchers/issues/933 diff --git a/lib/shoulda/matchers/active_record.rb b/lib/shoulda/matchers/active_record.rb index 7ed16585..4864b985 100644 --- a/lib/shoulda/matchers/active_record.rb +++ b/lib/shoulda/matchers/active_record.rb @@ -6,6 +6,8 @@ require "shoulda/matchers/active_record/association_matchers/join_table_matcher" require "shoulda/matchers/active_record/association_matchers/order_matcher" require "shoulda/matchers/active_record/association_matchers/through_matcher" require "shoulda/matchers/active_record/association_matchers/dependent_matcher" +require "shoulda/matchers/active_record/association_matchers/required_matcher" +require "shoulda/matchers/active_record/association_matchers/optional_matcher" require "shoulda/matchers/active_record/association_matchers/source_matcher" require "shoulda/matchers/active_record/association_matchers/model_reflector" require "shoulda/matchers/active_record/association_matchers/model_reflection" diff --git a/lib/shoulda/matchers/active_record/association_matcher.rb b/lib/shoulda/matchers/active_record/association_matcher.rb index 6489da90..fd470495 100644 --- a/lib/shoulda/matchers/active_record/association_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matcher.rb @@ -255,6 +255,48 @@ module Shoulda # # @return [AssociationMatcher] # + # ##### required + # + # Use `required` to assert that the association is not allowed to be nil. + # (Enabled by default in Rails 5+.) + # + # class Person < ActiveRecord::Base + # belongs_to :organization, required: true + # end + # + # # RSpec + # describe Person + # it { should belong_to(:organization).required } + # end + # + # # Minitest (Shoulda) + # class PersonTest < ActiveSupport::TestCase + # should belong_to(:organization).required + # end + # + # @return [AssociationMatcher] + # + # ##### optional + # + # Use `optional` to assert that the association is allowed to be nil. + # (Rails 5+ only.) + # + # class Person < ActiveRecord::Base + # belongs_to :organization, optional: true + # end + # + # # RSpec + # describe Person + # it { should belong_to(:organization).optional } + # end + # + # # Minitest (Shoulda) + # class PersonTest < ActiveSupport::TestCase + # should belong_to(:organization).optional + # end + # + # @return [AssociationMatcher] + # def belong_to(name) AssociationMatcher.new(:belongs_to, name) end @@ -714,6 +756,25 @@ module Shoulda # should have_one(:bank).autosave(true) # end # + # ##### required + # + # Use `required` to assert that the association is not allowed to be nil. + # (Rails 5+ only.) + # + # class Person < ActiveRecord::Base + # has_one :brain, required: true + # end + # + # # RSpec + # describe Person + # it { should have_one(:brain).required } + # end + # + # # Minitest (Shoulda) + # class PersonTest < ActiveSupport::TestCase + # should have_one(:brain).required + # end + # # @return [AssociationMatcher] # def have_one(name) @@ -891,42 +952,67 @@ module Shoulda @options = {} @submatchers = [] @missing = '' + + if macro == :belongs_to + if RailsShim.active_record_gte_5? + required + else + optional + end + end end def through(through) - through_matcher = AssociationMatchers::ThroughMatcher.new(through, name) - add_submatcher(through_matcher) + add_submatcher( + AssociationMatchers::ThroughMatcher, + through, + name, + ) self end def dependent(dependent) - dependent_matcher = AssociationMatchers::DependentMatcher.new(dependent, name) - add_submatcher(dependent_matcher) + add_submatcher( + AssociationMatchers::DependentMatcher, + dependent, + name, + ) self end def order(order) - order_matcher = AssociationMatchers::OrderMatcher.new(order, name) - add_submatcher(order_matcher) + add_submatcher( + AssociationMatchers::OrderMatcher, + order, + name, + ) self end def counter_cache(counter_cache = true) - counter_cache_matcher = AssociationMatchers::CounterCacheMatcher.new(counter_cache, name) - add_submatcher(counter_cache_matcher) + add_submatcher( + AssociationMatchers::CounterCacheMatcher, + counter_cache, + name, + ) self end def inverse_of(inverse_of) - inverse_of_matcher = - AssociationMatchers::InverseOfMatcher.new(inverse_of, name) - add_submatcher(inverse_of_matcher) + add_submatcher( + AssociationMatchers::InverseOfMatcher, + inverse_of, + name, + ) self end def source(source) - source_matcher = AssociationMatchers::SourceMatcher.new(source, name) - add_submatcher(source_matcher) + add_submatcher( + AssociationMatchers::SourceMatcher, + source, + name, + ) self end @@ -955,6 +1041,26 @@ module Shoulda self end + def required(required = true) + remove_submatcher(AssociationMatchers::OptionalMatcher) + add_submatcher( + AssociationMatchers::RequiredMatcher, + name, + required, + ) + self + end + + def optional(optional = true) + remove_submatcher(AssociationMatchers::RequiredMatcher) + add_submatcher( + AssociationMatchers::OptionalMatcher, + name, + optional, + ) + self + end + def validate(validate = true) @options[:validate] = validate self @@ -1016,8 +1122,15 @@ module Shoulda @reflector ||= AssociationMatchers::ModelReflector.new(subject, name) end - def add_submatcher(matcher) - @submatchers << matcher + def add_submatcher(matcher_class, *args) + remove_submatcher(matcher_class) + submatchers << matcher_class.new(*args) + end + + def remove_submatcher(matcher_class) + submatchers.delete_if do |submatcher| + submatcher.is_a?(matcher_class) + end end def macro_description @@ -1039,7 +1152,7 @@ module Shoulda def missing_options missing_options = [missing, missing_options_for_failing_submatchers] - missing_options.flatten.compact.join(', ') + missing_options.flatten.select(&:present?).join(', ') end def failing_submatchers diff --git a/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb new file mode 100644 index 00000000..83bef57f --- /dev/null +++ b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb @@ -0,0 +1,47 @@ +module Shoulda + module Matchers + module ActiveRecord + module AssociationMatchers + # @private + class OptionalMatcher + attr_reader :missing_option + + def initialize(attribute_name, optional) + @attribute_name = attribute_name + @missing_option = '' + @submatcher = submatcher_class_for(optional).new(nil). + for(attribute_name). + with_message(:required) + end + + def description + 'required: true' + end + + def matches?(subject) + if submatcher.matches?(subject) + true + else + @missing_option = + 'the association should have been defined ' + + 'with `optional: true`, but was not' + false + end + end + + private + + attr_reader :subject, :submatcher + + def submatcher_class_for(optional) + if optional + ActiveModel::AllowValueMatcher + else + ActiveModel::DisallowValueMatcher + end + end + end + end + end + end +end diff --git a/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb new file mode 100644 index 00000000..54e29645 --- /dev/null +++ b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb @@ -0,0 +1,50 @@ +module Shoulda + module Matchers + module ActiveRecord + module AssociationMatchers + # @private + class RequiredMatcher + attr_reader :missing_option + + def initialize(attribute_name, required) + @missing_option = '' + @submatcher = submatcher_class_for(required).new(nil). + for(attribute_name). + with_message(validation_message_key) + end + + def description + 'required: true' + end + + def matches?(subject) + if submatcher.matches?(subject) + true + else + @missing_option = + 'the association should have been defined ' + + 'with `required: true`, but was not' + false + end + end + + private + + attr_reader :subject, :submatcher + + def submatcher_class_for(required) + if required + ActiveModel::DisallowValueMatcher + else + ActiveModel::AllowValueMatcher + end + end + + def validation_message_key + RailsShim.validation_message_key_for_association_required_option + end + end + end + end + end +end diff --git a/lib/shoulda/matchers/rails_shim.rb b/lib/shoulda/matchers/rails_shim.rb index c5be6a30..c44decbd 100644 --- a/lib/shoulda/matchers/rails_shim.rb +++ b/lib/shoulda/matchers/rails_shim.rb @@ -21,8 +21,12 @@ module Shoulda Gem::Version.new('0') end - def active_record_major_version - ::ActiveRecord::VERSION::MAJOR + def active_record_gte_5? + Gem::Requirement.new('>= 5').satisfied_by?(active_record_version) + end + + def active_record_version + Gem::Version.new(::ActiveRecord::VERSION::STRING) rescue NameError Gem::Version.new('0') end @@ -92,7 +96,7 @@ module Shoulda end def tables_and_views(connection) - if active_record_major_version >= 5 + if active_record_gte_5? connection.data_sources else connection.tables @@ -107,6 +111,14 @@ module Shoulda end end + def validation_message_key_for_association_required_option + if active_record_gte_5? + :required + else + :blank + end + end + private def simply_generate_validation_message( diff --git a/spec/support/unit/helpers/active_record_versions.rb b/spec/support/unit/helpers/active_record_versions.rb index cf66604b..1f4cb299 100644 --- a/spec/support/unit/helpers/active_record_versions.rb +++ b/spec/support/unit/helpers/active_record_versions.rb @@ -32,5 +32,9 @@ module UnitTests def active_record_uniqueness_supports_array_columns? active_record_version < 5 end + + def active_record_supports_required_for_associations? + active_record_version >= 5 + end end end diff --git a/spec/support/unit/matchers/fail_with_message_matcher.rb b/spec/support/unit/matchers/fail_with_message_matcher.rb index 1a852356..56436f0c 100644 --- a/spec/support/unit/matchers/fail_with_message_matcher.rb +++ b/spec/support/unit/matchers/fail_with_message_matcher.rb @@ -24,13 +24,15 @@ module UnitTests lines << Shoulda::Matchers::Util.indent(expected, 2) if @actual + diff = differ.diff(@actual, expected)[1..-1] + lines << 'Actually failed with:' lines << Shoulda::Matchers::Util.indent(@actual, 2) - lines << "Diff:" - lines << Shoulda::Matchers::Util.indent( - differ.diff(@actual, expected)[1..-1], - 2 - ) + + if diff + lines << 'Diff:' + lines << Shoulda::Matchers::Util.indent(diff, 2) + end else lines << 'However, the expectation did not fail at all.' end diff --git a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb index f383487a..1a9d23f1 100644 --- a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb @@ -82,18 +82,18 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end it 'accepts an association with a valid :inverse_of option' do - expect(belonging_to_parent(inverse_of: :children)) - .to belong_to(:parent).inverse_of(:children) + expect(belonging_to_with_inverse(:parent, :children)). + to belong_to(:parent).inverse_of(:children) end it 'rejects an association with a bad :inverse_of option' do - expect(belonging_to_parent(inverse_of: :other_children)) - .not_to belong_to(:parent).inverse_of(:children) + expect(belonging_to_with_inverse(:parent, :other_children)). + not_to belong_to(:parent).inverse_of(:children) end it 'rejects an association that has no :inverse_of option' do - expect(belonging_to_parent) - .not_to belong_to(:parent).inverse_of(:children) + expect(belonging_to_parent). + not_to belong_to(:parent).inverse_of(:children) end it 'accepts an association with a valid :conditions option' do @@ -279,13 +279,92 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end - def belonging_to_parent(options = {}) - define_model :parent + context 'given an association with neither :required nor :optional specified' do + if active_record_supports_required_for_associations? + it 'assumes it is required' do + expect(belonging_to_parent).to belong_to(:parent).required + end + else + it 'assumes it is optional' do + expect(belonging_to_parent).to belong_to(:parent).optional + end + end + end + + context 'given an association with a matching :required option' do + it 'passes' do + expect(belonging_to_parent(required: true)). + to belong_to(:parent).required + end + end + + context 'given an association with a non-matching :required option' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(belonging_to_parent(required: false)). + to belong_to(:parent).required + end + + message = + 'Expected Child to have a belongs_to association called parent ' + + '(the association should have been defined with `required: true`, ' + + 'but was not)' + + expect(&assertion).to fail_with_message(message) + end + end + + if active_record_supports_required_for_associations? + context 'given an association with a matching :optional option' do + it 'passes' do + expect(belonging_to_parent(optional: true)). + to belong_to(:parent).optional + end + end + + context 'given an association with a non-matching :optional option' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(belonging_to_parent(optional: false)). + to belong_to(:parent).optional + end + + message = + 'Expected Child to have a belongs_to association called parent ' + + '(the association should have been defined with `optional: ' + + 'true`, but was not)' + + expect(&assertion).to fail_with_message(message) + end + end + end + + def belonging_to_parent(options = {}, parent_options = {}) + define_model :parent, parent_options define_model :child, parent_id: :integer do belongs_to :parent, options end.new end + def belonging_to_with_inverse(association, inverse_association) + parent_model_name = association.to_s.singularize + child_model_name = inverse_association.to_s.singularize + parent_foreign_key = "#{parent_model_name}_id" + + define_model parent_model_name do + has_many inverse_association + end + + child_model = define_model( + child_model_name, + parent_foreign_key => :integer, + ) do + belongs_to association, inverse_of: inverse_association + end + + child_model.new + end + def belonging_to_non_existent_class(model_name, assoc_name, options = {}) define_model model_name, "#{assoc_name}_id" => :integer do belongs_to assoc_name, options @@ -826,6 +905,31 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end + if active_record_supports_required_for_associations? + context 'given an association with a matching :required option' do + it 'passes' do + expect(having_one_detail(required: true)). + to have_one(:detail).required + end + end + end + + context 'given an association with a non-matching :required option' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(having_one_detail(required: false)). + to have_one(:detail).required + end + + message = + 'Expected Person to have a has_one association called detail ' + + '(the association should have been defined with `required: true`, ' + + 'but was not)' + + expect(&assertion).to fail_with_message(message) + end + end + def having_one_detail(options = {}) define_model :detail, person_id: :integer define_model(:person).tap do |model|