From 81d3a5899ea5b0efccd14ae256cd09ada0b538a7 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sat, 4 Sep 2021 09:24:05 +0300 Subject: [PATCH 1/4] Fix diff for compound when transforming actual Previously, we were passing the untransformed actual to the differ. Now, we take it from the matchers. fixes #1317 --- Changelog.md | 5 + lib/rspec/expectations/fail_with.rb | 2 +- lib/rspec/matchers.rb | 2 +- lib/rspec/matchers/built_in/compound.rb | 4 +- ...ultiple_diffs.rb => multi_matcher_diff.rb} | 32 +++---- spec/rspec/matchers/built_in/compound_spec.rb | 92 +++++++++++++++++++ ...ffs_spec.rb => multi_matcher_diff_spec.rb} | 27 +++--- 7 files changed, 132 insertions(+), 32 deletions(-) rename lib/rspec/matchers/{expecteds_for_multiple_diffs.rb => multi_matcher_diff.rb} (71%) rename spec/rspec/matchers/{expecteds_for_multiple_diffs_spec.rb => multi_matcher_diff_spec.rb} (85%) diff --git a/Changelog.md b/Changelog.md index 60ce3150e..caef450ba 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,11 @@ Enhancements: * Update `eq` and `eql` matchers to better highlight difference in string encoding. (Alan Foster, #1425) +Bug Fixes: + +* Fix the diff for redefined `actual` and reassigned `@actual` in compound + expectations failure messages. (Phil Pirozhkov, #1440) + ### 3.12.3 / 2023-04-20 [Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.12.2...v3.12.3) diff --git a/lib/rspec/expectations/fail_with.rb b/lib/rspec/expectations/fail_with.rb index 212e71c42..17b66b3d6 100644 --- a/lib/rspec/expectations/fail_with.rb +++ b/lib/rspec/expectations/fail_with.rb @@ -30,7 +30,7 @@ def fail_with(message, expected=nil, actual=nil) "appropriate failure_message[_when_negated] method to return a string?" end - message = ::RSpec::Matchers::ExpectedsForMultipleDiffs.from(expected).message_with_diff(message, differ, actual) + message = ::RSpec::Matchers::MultiMatcherDiff.from(expected, actual).message_with_diff(message, differ) RSpec::Support.notify_failure(RSpec::Expectations::ExpectationNotMetError.new message) end diff --git a/lib/rspec/matchers.rb b/lib/rspec/matchers.rb index 6a853ddf1..f0e6cf8ac 100644 --- a/lib/rspec/matchers.rb +++ b/lib/rspec/matchers.rb @@ -10,7 +10,7 @@ dsl matcher_delegator aliased_matcher - expecteds_for_multiple_diffs + multi_matcher_diff ].each { |file| RSpec::Support.require_rspec_matchers(file) } # RSpec's top level namespace. All of rspec-expectations is contained diff --git a/lib/rspec/matchers/built_in/compound.rb b/lib/rspec/matchers/built_in/compound.rb index 74f8ccd2e..0d8090c68 100644 --- a/lib/rspec/matchers/built_in/compound.rb +++ b/lib/rspec/matchers/built_in/compound.rb @@ -51,10 +51,10 @@ def diffable? end # @api private - # @return [RSpec::Matchers::ExpectedsForMultipleDiffs] + # @return [RSpec::Matchers::MultiMatcherDiff] def expected return nil unless evaluator - ::RSpec::Matchers::ExpectedsForMultipleDiffs.for_many_matchers(diffable_matcher_list) + ::RSpec::Matchers::MultiMatcherDiff.for_many_matchers(diffable_matcher_list) end protected diff --git a/lib/rspec/matchers/expecteds_for_multiple_diffs.rb b/lib/rspec/matchers/multi_matcher_diff.rb similarity index 71% rename from lib/rspec/matchers/expecteds_for_multiple_diffs.rb rename to lib/rspec/matchers/multi_matcher_diff.rb index b286bb5ae..cd2264e23 100644 --- a/lib/rspec/matchers/expecteds_for_multiple_diffs.rb +++ b/lib/rspec/matchers/multi_matcher_diff.rb @@ -1,9 +1,9 @@ module RSpec module Matchers # @api private - # Handles list of expected values when there is a need to render - # multiple diffs. Also can handle one value. - class ExpectedsForMultipleDiffs + # Handles list of expected and actual value pairs when there is a need + # to render multiple diffs. Also can handle one pair. + class MultiMatcherDiff # @private # Default diff label when there is only one matcher in diff # output @@ -19,22 +19,23 @@ def initialize(expected_list) # @api private # Wraps provided expected value in instance of - # ExpectedForMultipleDiffs. If provided value is already an - # ExpectedForMultipleDiffs then it just returns it. + # MultiMatcherDiff. If provided value is already an + # MultiMatcherDiff then it just returns it. # @param [Any] expected value to be wrapped - # @return [RSpec::Matchers::ExpectedsForMultipleDiffs] - def self.from(expected) + # @param [Any] actual value + # @return [RSpec::Matchers::MultiMatcherDiff] + def self.from(expected, actual) return expected if self === expected - new([[expected, DEFAULT_DIFF_LABEL]]) + new([[expected, DEFAULT_DIFF_LABEL, actual]]) end # @api private # Wraps provided matcher list in instance of - # ExpectedForMultipleDiffs. + # MultiMatcherDiff. # @param [Array] matchers list of matchers to wrap - # @return [RSpec::Matchers::ExpectedsForMultipleDiffs] + # @return [RSpec::Matchers::MultiMatcherDiff] def self.for_many_matchers(matchers) - new(matchers.map { |m| [m.expected, diff_label_for(m)] }) + new(matchers.map { |m| [m.expected, diff_label_for(m), m.actual] }) end # @api private @@ -42,10 +43,9 @@ def self.for_many_matchers(matchers) # factory and actual value if there are any # @param [String] message original failure message # @param [Proc] differ - # @param [Any] actual value # @return [String] - def message_with_diff(message, differ, actual) - diff = diffs(differ, actual) + def message_with_diff(message, differ) + diff = diffs(differ) message = "#{message}\n#{diff}" unless diff.empty? message end @@ -65,8 +65,8 @@ def truncated(description) end end - def diffs(differ, actual) - @expected_list.map do |(expected, diff_label)| + def diffs(differ) + @expected_list.map do |(expected, diff_label, actual)| diff = differ.diff(actual, expected) next if diff.strip.empty? if diff == "\e[0m\n\e[0m" diff --git a/spec/rspec/matchers/built_in/compound_spec.rb b/spec/rspec/matchers/built_in/compound_spec.rb index 82a8ead7b..6fc1eb3ae 100644 --- a/spec/rspec/matchers/built_in/compound_spec.rb +++ b/spec/rspec/matchers/built_in/compound_spec.rb @@ -543,6 +543,98 @@ def expect_block expect(error.message).to include(expected_failure) end end + + context 'when matcher transforms the actual' do + context 'when the matcher redefines `actual`' do + matcher :eq_downcase do |expected| + match do |actual| + @matcher_internal_actual = actual.downcase + values_match? expected, @matcher_internal_actual + end + + def actual + @matcher_internal_actual + end + + diffable + end + + it 'shows the redefined value in diff' do + expected_failure = + dedent(<<-EOS) + | expected "HELLO\\nWORLD" to eq downcase "bonjour\\nmonde" + | + |...and: + | + | expected "HELLO\\nWORLD" to eq downcase "hola\\nmon" + |Diff for (eq downcase "bonjour\\nmonde"): + |@@ -1,3 +1,3 @@ + |-bonjour + |-monde + |+hello + |+world + | + |Diff for (eq downcase "hola\\nmon"): + |@@ -1,3 +1,3 @@ + |-hola + |-mon + |+hello + |+world + EOS + + expect { + expect("HELLO\nWORLD") + .to eq_downcase("bonjour\nmonde") + .and eq_downcase("hola\nmon") + }.to fail do |error| + expect(error.message).to include(expected_failure) + end + end + end + + context 'when the matcher reassigns `@actual`' do + matcher :eq_downcase do |expected| + match do |actual| + @actual = actual.downcase + values_match? expected, @actual + end + + diffable + end + + it 'shows the reassigned value in diff' do + expected_failure = + dedent(<<-EOS) + | expected "hello\\nworld" to eq downcase "bonjour\\nmonde" + | + |...and: + | + | expected "hello\\nworld" to eq downcase "hola\\nmon" + |Diff for (eq downcase "bonjour\\nmonde"): + |@@ -1,3 +1,3 @@ + |-bonjour + |-monde + |+hello + |+world + | + |Diff for (eq downcase "hola\\nmon"): + |@@ -1,3 +1,3 @@ + |-hola + |-mon + |+hello + |+world + EOS + + expect { + expect("HELLO\nWORLD") + .to eq_downcase("bonjour\nmonde") + .and eq_downcase("hola\nmon") + }.to fail do |error| + expect(error.message).to include(expected_failure) + end + end + end + end end context "when both matchers are not diffable" do diff --git a/spec/rspec/matchers/expecteds_for_multiple_diffs_spec.rb b/spec/rspec/matchers/multi_matcher_diff_spec.rb similarity index 85% rename from spec/rspec/matchers/expecteds_for_multiple_diffs_spec.rb rename to spec/rspec/matchers/multi_matcher_diff_spec.rb index 222feaf6c..16c1c4576 100644 --- a/spec/rspec/matchers/expecteds_for_multiple_diffs_spec.rb +++ b/spec/rspec/matchers/multi_matcher_diff_spec.rb @@ -1,9 +1,9 @@ module RSpec module Matchers - RSpec.describe ExpectedsForMultipleDiffs do + RSpec.describe MultiMatcherDiff do before do - stub_const("::RSpec::Matchers::ExpectedsForMultipleDiffs::DESCRIPTION_MAX_LENGTH", 30) + stub_const("::RSpec::Matchers::MultiMatcherDiff::DESCRIPTION_MAX_LENGTH", 30) end class FakeDiffer @@ -17,11 +17,12 @@ def self.diff(actual, expected) let(:message) { "a message" } let(:actual) { "actual value" } - let(:wrapped_value) { described_class.from("expected value") } + let(:wrapped_value) { described_class.from("expected value", actual) } def create_matcher(stubs) instance_double(BuiltIn::BaseMatcher, stubs.merge( :matches? => true, + :actual => actual, :failure_message => "" )) end @@ -35,12 +36,12 @@ def create_matcher(stubs) let(:matcher_with_long_description) { create_matcher(:description => long_description, :expected => "expected value") } describe ".from" do - it "wraps provided value in ExpectedsForMultipleDiffs" do + it "wraps provided value in MultiMatcherDiff" do expect(wrapped_value).to be_a(described_class) end it "returns original value if it was already wrapped" do - expect(described_class.from(wrapped_value)).to be(wrapped_value) + expect(described_class.from(wrapped_value, actual)).to be(wrapped_value) end end @@ -49,7 +50,7 @@ def create_matcher(stubs) it "has a diff for all matchers with their description" do expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message |Diff for (matcher 1 description):["actual value", "expected 1"] @@ -63,7 +64,7 @@ def create_matcher(stubs) it "returns a message warning if the diff is empty" do allow(FakeDiffer).to receive(:diff) { "\e[0m\n\e[0m" } expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message |Diff: @@ -74,7 +75,7 @@ def create_matcher(stubs) it "returns just provided message if diff is just whitespace" do allow(FakeDiffer).to receive(:diff) { " \n \t" } expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message EOS @@ -82,7 +83,7 @@ def create_matcher(stubs) it "returns regular message with diff when single expected" do expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message |Diff:["actual value", "expected value"] @@ -90,10 +91,12 @@ def create_matcher(stubs) end it "returns message with diff and matcher description when single expected with matcher" do - wrapped_value = described_class.for_many_matchers([include("expected value")]) + matcher = include("expected value") + matcher.matches?(actual) + wrapped_value = described_class.for_many_matchers([matcher]) expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message |Diff for (include "expected value"):["actual value", ["expected value"]] @@ -104,7 +107,7 @@ def create_matcher(stubs) wrapped_value = described_class.for_many_matchers([matcher_with_long_description]) expect(wrapped_value.message_with_diff( - message, differ, actual + message, differ )).to eq(dedent <<-EOS) |a message |Diff for (#{truncated_description}):["actual value", "expected value"] From e1da673093c252abee78469ca89cb7aa00d7e6c9 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Fri, 24 Feb 2023 18:14:39 +0300 Subject: [PATCH 2/4] Add a spec for #1406 --- spec/rspec/matchers/built_in/output_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/rspec/matchers/built_in/output_spec.rb b/spec/rspec/matchers/built_in/output_spec.rb index 07f9e8979..7ea95f6bb 100644 --- a/spec/rspec/matchers/built_in/output_spec.rb +++ b/spec/rspec/matchers/built_in/output_spec.rb @@ -194,5 +194,26 @@ def print_to_stream(msg) expect(output("foo").description).to eq('output "foo" to some stream') end end + + RSpec.describe "can capture stdin and stderr" do + it "prints diff for both when both fail" do + expect { + expect { print "foo"; $stderr.print("bar") } + .to output(/baz/).to_stdout + .and output(/qux/).to_stderr + } + .to fail_including( + 'expected block to output /baz/ to stdout, but output "foo"', + '...and:', + 'expected block to output /qux/ to stderr, but output "bar"', + 'Diff for (output /baz/ to stdout):', + '-/baz/', + '+"foo"', + 'Diff for (output /qux/ to stderr):', + '-/qux/', + '+"bar"' + ) + end + end end end From 3c52f5a0fde5be068bfc095d5e41a00e7893a2da Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Wed, 20 Dec 2023 20:25:01 +0000 Subject: [PATCH 3/4] Update spec/rspec/matchers/built_in/output_spec.rb --- spec/rspec/matchers/built_in/output_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/rspec/matchers/built_in/output_spec.rb b/spec/rspec/matchers/built_in/output_spec.rb index 7ea95f6bb..f87e4d25f 100644 --- a/spec/rspec/matchers/built_in/output_spec.rb +++ b/spec/rspec/matchers/built_in/output_spec.rb @@ -201,8 +201,7 @@ def print_to_stream(msg) expect { print "foo"; $stderr.print("bar") } .to output(/baz/).to_stdout .and output(/qux/).to_stderr - } - .to fail_including( + }.to fail_including( 'expected block to output /baz/ to stdout, but output "foo"', '...and:', 'expected block to output /qux/ to stderr, but output "bar"', From 9b44765254fcbccc8708137379320a437b9740da Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Wed, 20 Dec 2023 20:43:29 +0000 Subject: [PATCH 4/4] Apply suggestions from code review More 1.8.7 fixes --- spec/rspec/matchers/built_in/compound_spec.rb | 12 ++++++------ spec/rspec/matchers/built_in/output_spec.rb | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/rspec/matchers/built_in/compound_spec.rb b/spec/rspec/matchers/built_in/compound_spec.rb index 6fc1eb3ae..2fc72fcf4 100644 --- a/spec/rspec/matchers/built_in/compound_spec.rb +++ b/spec/rspec/matchers/built_in/compound_spec.rb @@ -583,9 +583,9 @@ def actual EOS expect { - expect("HELLO\nWORLD") - .to eq_downcase("bonjour\nmonde") - .and eq_downcase("hola\nmon") + expect( + "HELLO\nWORLD" + ).to eq_downcase("bonjour\nmonde").and eq_downcase("hola\nmon") }.to fail do |error| expect(error.message).to include(expected_failure) end @@ -626,9 +626,9 @@ def actual EOS expect { - expect("HELLO\nWORLD") - .to eq_downcase("bonjour\nmonde") - .and eq_downcase("hola\nmon") + expect( + "HELLO\nWORLD" + ).to eq_downcase("bonjour\nmonde").and eq_downcase("hola\nmon") }.to fail do |error| expect(error.message).to include(expected_failure) end diff --git a/spec/rspec/matchers/built_in/output_spec.rb b/spec/rspec/matchers/built_in/output_spec.rb index f87e4d25f..7e90b83db 100644 --- a/spec/rspec/matchers/built_in/output_spec.rb +++ b/spec/rspec/matchers/built_in/output_spec.rb @@ -198,9 +198,9 @@ def print_to_stream(msg) RSpec.describe "can capture stdin and stderr" do it "prints diff for both when both fail" do expect { - expect { print "foo"; $stderr.print("bar") } - .to output(/baz/).to_stdout - .and output(/qux/).to_stderr + expect { + print "foo"; $stderr.print("bar") + }.to output(/baz/).to_stdout.and output(/qux/).to_stderr }.to fail_including( 'expected block to output /baz/ to stdout, but output "foo"', '...and:',