From 441fa94a8bc102de20c54150e872856e67d5b713 Mon Sep 17 00:00:00 2001 From: Marc-Andre Lafortune Date: Wed, 17 Jun 2020 17:13:42 -0400 Subject: [PATCH] Add setting for strict_predicate_matchers. Make failure message more precise --- Changelog.md | 3 + features/built_in_matchers/predicates.feature | 50 ++++++++++++-- features/test_frameworks/minitest.feature | 4 +- lib/rspec/expectations/configuration.rb | 16 +++++ lib/rspec/matchers/built_in/has.rb | 22 ++++-- spec/rspec/matchers/built_in/be_spec.rb | 68 ++++++++++++++++++- spec/rspec/matchers/built_in/has_spec.rb | 24 +++++-- spec/spec_helper.rb | 1 + 8 files changed, 167 insertions(+), 21 deletions(-) diff --git a/Changelog.md b/Changelog.md index 99a5ca8c9..cd7150c38 100644 --- a/Changelog.md +++ b/Changelog.md @@ -29,6 +29,9 @@ Enhancements: (Jon Rowe, #1169) * Dynamic `have_` matchers now have output consistent with other dynamic matchers. (Marc-André Lafortune, #1195) +* New config option `strict_predicate_matchers` allows predicate matcher to be strict + (i.e. match for `true` or `false`) instead of the default (match truthy vs `false` or `nil`). + (Marc-André Lafortune, #1196) ### 3.9.2 / 2020-05-08 [Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.9.1...v3.9.2) diff --git a/features/built_in_matchers/predicates.feature b/features/built_in_matchers/predicates.feature index 317ffeec8..0e0256a6c 100644 --- a/features/built_in_matchers/predicates.feature +++ b/features/built_in_matchers/predicates.feature @@ -39,7 +39,7 @@ Feature: Predicate matchers In either case, RSpec provides nice, clear error messages, such as: - `expected zero? to return true, got false` + `expected zero? to be truthy, got false` Calling private methods will also fail: @@ -60,7 +60,7 @@ Feature: Predicate matchers """ When I run `rspec should_be_zero_spec.rb` Then the output should contain "2 examples, 1 failure" - And the output should contain "expected `7.zero?` to return true, got false" + And the output should contain "expected `7.zero?` to be truthy, got false" Scenario: should_not be_empty (based on Array#empty?) Given a file named "should_not_be_empty_spec.rb" with: @@ -75,7 +75,7 @@ Feature: Predicate matchers """ When I run `rspec should_not_be_empty_spec.rb` Then the output should contain "2 examples, 1 failure" - And the output should contain "expected `[].empty?` to return false, got true" + And the output should contain "expected `[].empty?` to be falsey, got true" Scenario: should have_key (based on Hash#has_key?) Given a file named "should_have_key_spec.rb" with: @@ -88,7 +88,7 @@ Feature: Predicate matchers """ When I run `rspec should_have_key_spec.rb` Then the output should contain "2 examples, 1 failure" - And the output should contain "expected `{:foo=>7}.has_key?(:bar)` to return true, got false" + And the output should contain "expected `{:foo=>7}.has_key?(:bar)` to be truthy, got false" Scenario: should_not have_all_string_keys (based on custom #has_all_string_keys? method) Given a file named "should_not_have_all_string_keys_spec.rb" with: @@ -114,7 +114,7 @@ Feature: Predicate matchers """ When I run `rspec should_not_have_all_string_keys_spec.rb` Then the output should contain "2 examples, 1 failure" - And the output should contain "expected `42.0.has_decimals?` to return true, got false" + And the output should contain "expected `42.0.has_decimals?` to be truthy, got false" Scenario: matcher arguments are passed on to the predicate method Given a file named "predicate_matcher_argument_spec.rb" with: @@ -136,8 +136,44 @@ Feature: Predicate matchers """ When I run `rspec predicate_matcher_argument_spec.rb` Then the output should contain "4 examples, 2 failures" - And the output should contain "expected `12.multiple_of?(4)` to return false, got true" - And the output should contain "expected `12.multiple_of?(5)` to return true, got false" + And the output should contain "expected `12.multiple_of?(4)` to be falsey, got true" + And the output should contain "expected `12.multiple_of?(5)` to be truthy, got false" + + Scenario: the config `strict_predicate_matchers` impacts matching of results other than `true` and `false` + Given a file named "strict_or_not.rb" with: + """ruby + class StrangeResult + def has_strange_result? + 42 + end + end + + RSpec.describe StrangeResult do + subject { StrangeResult.new } + + before do + RSpec.configure do |config| + config.expect_with :rspec do |expectations| + expectations.strict_predicate_matchers = strict + end + end + end + + context 'with non-strict matchers (default)' do + let(:strict) { false } + it { is_expected.to have_strange_result } + end + + context 'with strict matchers' do + let(:strict) { true } + # deliberate failure + it { is_expected.to have_strange_result } + end + end + """ + When I run `rspec strict_or_not.rb` + Then the output should contain "2 examples, 1 failure" + And the output should contain "has_strange_result?` to return true, got 42" Scenario: calling private method with be_predicate causes error Given a file named "attempting_to_match_private_method_spec.rb" with: diff --git a/features/test_frameworks/minitest.feature b/features/test_frameworks/minitest.feature index 687a82be0..164ef841f 100644 --- a/features/test_frameworks/minitest.feature +++ b/features/test_frameworks/minitest.feature @@ -47,7 +47,7 @@ Feature: Minitest integration """ When I run `ruby rspec_expectations_test.rb` Then the output should contain "4 runs, 5 assertions, 2 failures, 0 errors" - And the output should contain "expected `[1, 2].empty?` to return true, got false" + And the output should contain "expected `[1, 2].empty?` to be truthy, got false" And the output should contain "be_an_int is deprecated" And the output should contain "Got 2 failures from failure aggregation block" @@ -105,7 +105,7 @@ Feature: Minitest integration """ When I run `ruby rspec_expectations_spec.rb` Then the output should contain "9 runs, 10 assertions, 5 failures, 0 errors" - And the output should contain "expected `[1, 2].empty?` to return true, got false" + And the output should contain "expected `[1, 2].empty?` to be truthy, got false" And the output should contain "expected ZeroDivisionError but nothing was raised" And the output should contain "Got 2 failures from failure aggregation block" And the output should contain "Expected [1, 2] to be empty?" diff --git a/lib/rspec/expectations/configuration.rb b/lib/rspec/expectations/configuration.rb index 377ced9dc..b4af830e1 100644 --- a/lib/rspec/expectations/configuration.rb +++ b/lib/rspec/expectations/configuration.rb @@ -28,6 +28,7 @@ class Configuration def initialize @on_potential_false_positives = :warn + @strict_predicate_matchers = false end # Configures the supported syntax. @@ -185,6 +186,21 @@ def on_potential_false_positives=(behavior) @on_potential_false_positives = behavior end + + # Configures RSpec to check predicate matchers to `be(true)` / `be(false)` (strict), + # or `be_truthy` / `be_falsey` (not strict). + # Historically, the default was `false`, but `true` is recommended. + def strict_predicate_matchers=(flag) + raise ArgumentError, "Pass `true` or `false`" unless flag == true || flag == false + @strict_predicate_matchers = flag + end + + attr_reader :strict_predicate_matchers + + def strict_predicate_matchers? + @strict_predicate_matchers + end + # Indicates what RSpec will do about matcher use which will # potentially cause false positives in tests, generally you want to # avoid such scenarios so this defaults to `true`. diff --git a/lib/rspec/matchers/built_in/has.rb b/lib/rspec/matchers/built_in/has.rb index 63623d02f..ead431b65 100644 --- a/lib/rspec/matchers/built_in/has.rb +++ b/lib/rspec/matchers/built_in/has.rb @@ -30,7 +30,7 @@ def matches?(actual, &block) def does_not_match?(actual, &block) @actual = actual @block ||= block - predicate_accessible? && !predicate_matches? + predicate_accessible? && predicate_matches?(false) end # @api private @@ -90,8 +90,12 @@ def predicate_method_name predicate end - def predicate_matches? - !!predicate_result + def predicate_matches?(value=true) + if RSpec::Expectations.configuration.strict_predicate_matchers? + value == predicate_result + else + value == !!predicate_result + end end def root @@ -108,7 +112,17 @@ def method_description def failure_message_expecting(value) validity_message || - "expected `#{actual_formatted}.#{predicate}#{args_to_s}` to return #{value}, got #{description_of @predicate_result}" + "expected `#{actual_formatted}.#{predicate}#{args_to_s}` to #{expectation_of value}, got #{description_of @predicate_result}" + end + + def expectation_of(value) + if RSpec::Expectations.configuration.strict_predicate_matchers? + "return #{value}" + elsif value + "be truthy" + else + "be falsey" + end end def validity_message diff --git a/spec/rspec/matchers/built_in/be_spec.rb b/spec/rspec/matchers/built_in/be_spec.rb index 0752524c3..985a51fa8 100644 --- a/spec/rspec/matchers/built_in/be_spec.rb +++ b/spec/rspec/matchers/built_in/be_spec.rb @@ -58,6 +58,35 @@ }.to fail_with("expected `#{actual.inspect}.happy?` to return true, got nil") end + context "when strict_predicate_matchers is set to true" do + it "fails when actual returns 42 for :predicate?" do + actual = double("actual", :happy? => 42) + expect { + expect(actual).to be_happy + }.to fail_with("expected `#{actual.inspect}.happy?` to return true, got 42") + end + end + + context "when strict_predicate_matchers is set to false" do + around do |example| + RSpec::Expectations.configuration.strict_predicate_matchers = false + example.run + RSpec::Expectations.configuration.strict_predicate_matchers = true + end + + it "passes when actual returns truthy value for :predicate?" do + actual = double("actual", :happy? => 42) + expect(actual).to be_happy + end + + it "states actual predicate used when it fails" do + actual = double("actual", :happy? => false) + expect { + expect(actual).to be_happy + }.to fail_with("expected `#{actual.inspect}.happy?` to be truthy, got false") + end + end + it "fails when actual does not respond to :predicate?" do expect { expect(Object.new).to be_happy @@ -184,14 +213,47 @@ def foo? end RSpec.describe "expect(...).not_to be_predicate" do + let(:strict_predicate_matchers) { true } + + around do |example| + default = RSpec::Expectations.configuration.strict_predicate_matchers? + RSpec::Expectations.configuration.strict_predicate_matchers = strict_predicate_matchers + example.run + RSpec::Expectations.configuration.strict_predicate_matchers = default + end + it "passes when actual returns false for :sym?" do actual = double("actual", :happy? => false) expect(actual).not_to be_happy end - it "passes when actual returns nil for :sym?" do - actual = double("actual", :happy? => nil) - expect(actual).not_to be_happy + context "when strict_predicate_matchers is set to true" do + it "fails when actual returns nil for :sym?" do + actual = double("actual", :happy? => nil) + expect { + expect(actual).not_to be_happy + }.to fail_with("expected `#{actual.inspect}.happy?` to return false, got nil") + end + end + + context "when strict_predicate_matchers is set to false" do + around do |example| + RSpec::Expectations.configuration.strict_predicate_matchers = false + example.run + RSpec::Expectations.configuration.strict_predicate_matchers = true + end + + it "passes when actual returns nil for :sym?" do + actual = double("actual", :happy? => nil) + expect(actual).not_to be_happy + end + + it "shows actual comparision made when it fails" do + actual = double("actual", :happy? => 42) + expect { + expect(actual).not_to be_happy + }.to fail_with("expected `#{actual.inspect}.happy?` to be falsey, got 42") + end end it "fails when actual returns true for :sym?" do diff --git a/spec/rspec/matchers/built_in/has_spec.rb b/spec/rspec/matchers/built_in/has_spec.rb index eba5cb54d..8a3bb5ca7 100644 --- a/spec/rspec/matchers/built_in/has_spec.rb +++ b/spec/rspec/matchers/built_in/has_spec.rb @@ -139,12 +139,26 @@ def o.has_sym?(sym); sym == :foo; end expect({ :a => "A" }).not_to have_key(:b) end - it "passes if #has_sym?(*args) returns nil" do - klass = Class.new do - def has_foo? - end + context "when strict_predicate_matchers is set to true" do + it "fails when #has_sym? returns nil" do + actual = double("actual", :has_foo? => nil) + expect { + expect(actual).not_to have_foo + }.to fail_with("expected `#{actual.inspect}.has_foo?` to return false, got nil") + end + end + + context "when strict_predicate_matchers is set to false" do + around do |example| + RSpec::Expectations.configuration.strict_predicate_matchers = false + example.run + RSpec::Expectations.configuration.strict_predicate_matchers = true + end + + it "passes if #has_sym?(*args) returns nil" do + actual = double("actual", :has_foo? => nil) + expect(actual).not_to have_foo end - expect(klass.new).not_to have_foo end it "fails if #has_sym?(*args) returns true" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 81769ab42..2686974a9 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -52,6 +52,7 @@ def hash_inspect(hash) $default_expectation_syntax = expectations.syntax # rubocop:disable Style/GlobalVars expectations.syntax = :expect expectations.include_chain_clauses_in_custom_matcher_descriptions = true + expectations.strict_predicate_matchers = true end config.mock_with :rspec do |mocks|