From edfc6051cfd453fb99efb3f89c872e3900031fcc Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Tue, 7 May 2024 19:25:12 -0400 Subject: [PATCH 1/7] Add dynamic matcher specs for be_/have_ vs spies Specifically, when applying these dynamic matchers to a spy, only consider them to have the method in question if it's explicitly defined on the double; don't let the null-object double pretend. --- spec/rspec/matchers/built_in/be_spec.rb | 15 +++++++++++++++ spec/rspec/matchers/built_in/has_spec.rb | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/spec/rspec/matchers/built_in/be_spec.rb b/spec/rspec/matchers/built_in/be_spec.rb index 42ba25ff4..b38469e7b 100644 --- a/spec/rspec/matchers/built_in/be_spec.rb +++ b/spec/rspec/matchers/built_in/be_spec.rb @@ -85,6 +85,21 @@ expect(actual).to be_happy }.to fail_with("expected `#{actual.inspect}.happy?` to be truthy, got false") end + + it "does not allow non-supplied dynamic matchers to pass on spies" do + thing = spy("thing", has_recv?: true) + + expect(thing).to have_recv(:foo) + expect(thing).to have_received(:has_recv?).with(:foo) + + begin + expect(thing).to have_receivex(:foo) + rescue RSpec::Expectations::ExpectationNotMetError + @reached = true + end + expect(@reached).to be_truthy + expect(thing).not_to have_received(:has_receivex?) + end end it "fails when actual does not respond to :predicate?" do diff --git a/spec/rspec/matchers/built_in/has_spec.rb b/spec/rspec/matchers/built_in/has_spec.rb index c392b12fa..37a2e9c2e 100644 --- a/spec/rspec/matchers/built_in/has_spec.rb +++ b/spec/rspec/matchers/built_in/has_spec.rb @@ -159,6 +159,21 @@ def o.has_sym?(sym); sym == :foo; end actual = double("actual", :has_foo? => nil) expect(actual).not_to have_foo end + + it "does not allow non-supplied dynamic matchers to pass on spies" do + thing = spy("thing", furg?: true) + + expect(thing).to be_furg(:foo) + expect(thing).to have_received(:furg?).with(:foo) + + begin + expect(thing).to be_blurm(:foo) + rescue RSpec::Expectations::ExpectationNotMetError + @reached = true + end + expect(@reached).to be_truthy + expect(thing).not_to have_received(:blurm?) + end end it "fails if #has_sym?(*args) returns true" do From 2f135bd340867d357997e909f5322eabdf40f4b3 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Tue, 7 May 2024 19:27:54 -0400 Subject: [PATCH 2/7] Don't let null-object spies fool dynamic matchers This should short-circuit a common mistake, where someone typos expect(my_spy).to have_receivedd(:foo) and gets an evergreen test, because strict_predicate_matchers is disabled, and null-object spies return a truthy value (themselves) when asked `has_receivedd?`. --- lib/rspec/matchers/built_in/has.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/rspec/matchers/built_in/has.rb b/lib/rspec/matchers/built_in/has.rb index 5036a6405..274647e08 100644 --- a/lib/rspec/matchers/built_in/has.rb +++ b/lib/rspec/matchers/built_in/has.rb @@ -46,8 +46,19 @@ def description private + # Catch a semi-frequent typo - if you have strict_predicate_matchers disabled and + # expect(spy).to have_receieveddd(:foo) it would be evergreen - the dynamic matcher + # queries `has_receiveddd?`, the spy _fakes_ the method, returning its (truthy) self. + def really_responds_to?(method) + if defined?(RSpec::Mocks::Double) && @actual.is_a?(RSpec::Mocks::Double) + @actual.respond_to?(method) && @actual.methods.include?(method) + else + @actual.respond_to?(method) + end + end + def predicate_accessible? - @actual.respond_to? predicate + really_responds_to?(predicate) end # support 1.8.7, evaluate once at load time for performance @@ -155,7 +166,7 @@ def failure_to_respond_explanation end def predicate_accessible? - super || actual.respond_to?(present_tense_predicate) + super || really_responds_to?(present_tense_predicate) end def present_tense_predicate From 990c666f2303e97e3e3292f1e0f6b73cb3877325 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Wed, 8 May 2024 07:10:19 -0400 Subject: [PATCH 3/7] Split the has_/be_ spy-specs and use fail-matcher --- spec/rspec/matchers/built_in/be_spec.rb | 17 ++++++++--------- spec/rspec/matchers/built_in/has_spec.rb | 17 ++++++++--------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/spec/rspec/matchers/built_in/be_spec.rb b/spec/rspec/matchers/built_in/be_spec.rb index b38469e7b..4e4852b47 100644 --- a/spec/rspec/matchers/built_in/be_spec.rb +++ b/spec/rspec/matchers/built_in/be_spec.rb @@ -86,19 +86,18 @@ }.to fail_with("expected `#{actual.inspect}.happy?` to be truthy, got false") end - it "does not allow non-supplied dynamic matchers to pass on spies" do + it "allows dynamic matchers to pass for supplied methods on spies" do thing = spy("thing", has_recv?: true) - expect(thing).to have_recv(:foo) expect(thing).to have_received(:has_recv?).with(:foo) + end - begin - expect(thing).to have_receivex(:foo) - rescue RSpec::Expectations::ExpectationNotMetError - @reached = true - end - expect(@reached).to be_truthy - expect(thing).not_to have_received(:has_receivex?) + it "does not allow dynamic matchers to pass for inferred methods on spies" do + thing = spy("thing") + expect { + expect(thing).to have_recv(:foo) + }.to fail + expect(thing).not_to have_received(:has_recv?) end end diff --git a/spec/rspec/matchers/built_in/has_spec.rb b/spec/rspec/matchers/built_in/has_spec.rb index 37a2e9c2e..e420bfdc7 100644 --- a/spec/rspec/matchers/built_in/has_spec.rb +++ b/spec/rspec/matchers/built_in/has_spec.rb @@ -160,19 +160,18 @@ def o.has_sym?(sym); sym == :foo; end expect(actual).not_to have_foo end - it "does not allow non-supplied dynamic matchers to pass on spies" do + it "allows dynamic matchers to pass for supplied methods on spies" do thing = spy("thing", furg?: true) - expect(thing).to be_furg(:foo) expect(thing).to have_received(:furg?).with(:foo) + end - begin - expect(thing).to be_blurm(:foo) - rescue RSpec::Expectations::ExpectationNotMetError - @reached = true - end - expect(@reached).to be_truthy - expect(thing).not_to have_received(:blurm?) + it "does not allow dynamic matchers to pass for inferred methods on spies" do + thing = spy("thing") + expect { + expect(thing).to be_furg(:foo) + }.to fail + expect(thing).not_to have_received(:furg?) end end From 46932beb5ed6b9aa6b77a3fa62c104d13d8f6be3 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Wed, 8 May 2024 07:32:14 -0400 Subject: [PATCH 4/7] Handle proxied BasicObjects in really_responds_to? BasicObjects don't even have `is_a?` (or `class`, or `===`, etc), so the only way I see to behave correctly for that case is to _try_ to check if they're a Double, and also handle `NoMethodError` with "Oh, you're that basic? Not a Double then." --- lib/rspec/matchers/built_in/has.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/rspec/matchers/built_in/has.rb b/lib/rspec/matchers/built_in/has.rb index 274647e08..8cea6a47d 100644 --- a/lib/rspec/matchers/built_in/has.rb +++ b/lib/rspec/matchers/built_in/has.rb @@ -55,6 +55,9 @@ def really_responds_to?(method) else @actual.respond_to?(method) end + rescue NoMethodError + # Proxied BasicObjects don't have `is_a?`, or basically anything else. + @actual.respond_to?(method) end def predicate_accessible? From 8d0ae41d3f029bc0bc9ff715f33dc7d8739256d2 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Wed, 8 May 2024 16:52:26 -0400 Subject: [PATCH 5/7] Avoid modern hash syntax, we support 1.8.7 --- spec/rspec/matchers/built_in/be_spec.rb | 2 +- spec/rspec/matchers/built_in/has_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/rspec/matchers/built_in/be_spec.rb b/spec/rspec/matchers/built_in/be_spec.rb index 4e4852b47..b4d3aff9d 100644 --- a/spec/rspec/matchers/built_in/be_spec.rb +++ b/spec/rspec/matchers/built_in/be_spec.rb @@ -87,7 +87,7 @@ end it "allows dynamic matchers to pass for supplied methods on spies" do - thing = spy("thing", has_recv?: true) + thing = spy("thing", :has_recv? => true) expect(thing).to have_recv(:foo) expect(thing).to have_received(:has_recv?).with(:foo) end diff --git a/spec/rspec/matchers/built_in/has_spec.rb b/spec/rspec/matchers/built_in/has_spec.rb index e420bfdc7..2b1709234 100644 --- a/spec/rspec/matchers/built_in/has_spec.rb +++ b/spec/rspec/matchers/built_in/has_spec.rb @@ -161,7 +161,7 @@ def o.has_sym?(sym); sym == :foo; end end it "allows dynamic matchers to pass for supplied methods on spies" do - thing = spy("thing", furg?: true) + thing = spy("thing", :furg? => true) expect(thing).to be_furg(:foo) expect(thing).to have_received(:furg?).with(:foo) end From 5cf5ad303dce2d01e2e451ee87762677c37db45b Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Thu, 9 May 2024 09:38:09 -0400 Subject: [PATCH 6/7] In 1.8.7, Object#methods returns an array of strings Luckily, we already have some code in this very file that handles that situation, so we can just ride along. --- lib/rspec/matchers/built_in/has.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/rspec/matchers/built_in/has.rb b/lib/rspec/matchers/built_in/has.rb index 8cea6a47d..91d971829 100644 --- a/lib/rspec/matchers/built_in/has.rb +++ b/lib/rspec/matchers/built_in/has.rb @@ -51,7 +51,7 @@ def description # queries `has_receiveddd?`, the spy _fakes_ the method, returning its (truthy) self. def really_responds_to?(method) if defined?(RSpec::Mocks::Double) && @actual.is_a?(RSpec::Mocks::Double) - @actual.respond_to?(method) && @actual.methods.include?(method) + @actual.respond_to?(method) && methods_include?(method) else @actual.respond_to?(method) end @@ -70,11 +70,19 @@ def predicate_accessible? def private_predicate? @actual.private_methods.include? predicate.to_s end + + def methods_include?(method) + @actual.methods.include?(method.to_s) + end # :nocov: else def private_predicate? @actual.private_methods.include? predicate end + + def methods_include?(method) + @actual.methods.include?(method) + end end def predicate_result From 6bd13ec83091cd8c507080fcdb13483c636ab59f Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 14 Jun 2024 11:20:34 -0400 Subject: [PATCH 7/7] Use conditionally-defined behavior instead, as suggested. Much nicer, and using === instead of is_a? also saves us the rescue clause! --- lib/rspec/matchers/built_in/has.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/rspec/matchers/built_in/has.rb b/lib/rspec/matchers/built_in/has.rb index 91d971829..ccd4d42f4 100644 --- a/lib/rspec/matchers/built_in/has.rb +++ b/lib/rspec/matchers/built_in/has.rb @@ -49,15 +49,18 @@ def description # Catch a semi-frequent typo - if you have strict_predicate_matchers disabled and # expect(spy).to have_receieveddd(:foo) it would be evergreen - the dynamic matcher # queries `has_receiveddd?`, the spy _fakes_ the method, returning its (truthy) self. - def really_responds_to?(method) - if defined?(RSpec::Mocks::Double) && @actual.is_a?(RSpec::Mocks::Double) - @actual.respond_to?(method) && methods_include?(method) - else + if defined?(RSpec::Mocks::Double) + def really_responds_to?(method) + if RSpec::Mocks::Double === @actual + @actual.respond_to?(method) && methods_include?(method) + else + @actual.respond_to?(method) + end + end + else + def really_responds_to?(method) @actual.respond_to?(method) end - rescue NoMethodError - # Proxied BasicObjects don't have `is_a?`, or basically anything else. - @actual.respond_to?(method) end def predicate_accessible?