From 0704fd2a4855f5743758765e875722d25b0d1c46 Mon Sep 17 00:00:00 2001 From: Michael Miller Date: Sat, 14 May 2022 23:30:15 -0600 Subject: [PATCH] Adjust evaluation order of change matcher expressions Handles reference types better and matches RSpec more closely. --- CHANGELOG.md | 1 + spec/rspec/core/explicit_subject_spec.cr | 6 +-- .../matchers/change_exact_matcher.cr | 49 ++++++++++--------- src/spectator/matchers/change_from_matcher.cr | 49 +++++++++++-------- src/spectator/matchers/change_to_matcher.cr | 34 ++++++------- 5 files changed, 73 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74b09f5..2f78cc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed - Forward example procsy `to_s` to underlying example. [#70](https://gitlab.com/arctic-fox/spectator/-/issues/70) +- Adjust evaluation order of `change` matcher expressions. ## [0.10.5] - 2022-01-27 ### Fixed diff --git a/spec/rspec/core/explicit_subject_spec.cr b/spec/rspec/core/explicit_subject_spec.cr index 825f6bf..3775b8a 100644 --- a/spec/rspec/core/explicit_subject_spec.cr +++ b/spec/rspec/core/explicit_subject_spec.cr @@ -34,16 +34,14 @@ Spectator.describe "Explicit Subject" do subject { @@element_list.pop } - skip "is memoized across calls (i.e. the block is invoked once)", - reason: "RSpec calls the \"actual\" block after the \"change block\"." do + it "is memoized across calls (i.e. the block is invoked once)" do expect do 3.times { subject } end.to change { @@element_list }.from([1, 2, 3]).to([1, 2]) expect(subject).to eq(3) end - skip "is not memoized across examples", - reason: "RSpec calls the \"actual\" block after the \"change block\"." do + it "is not memoized across examples" do expect { subject }.to change { @@element_list }.from([1, 2]).to([1]) expect(subject).to eq(2) end diff --git a/src/spectator/matchers/change_exact_matcher.cr b/src/spectator/matchers/change_exact_matcher.cr index 2f0880e..09d1a83 100644 --- a/src/spectator/matchers/change_exact_matcher.cr +++ b/src/spectator/matchers/change_exact_matcher.cr @@ -27,26 +27,32 @@ module Spectator::Matchers # Actually performs the test against the expression. def match(actual : Expression(T)) : MatchData forall T - before, after = change(actual) + before = expression.value + before_inspect = before.inspect + if expected_before == before - if before == after - FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label}", - before: before.inspect, - after: after.inspect - ) - elsif expected_after == after + actual.value # Trigger block that might cause a change. + after = expression.value + after_inspect = after.inspect + + if expected_after == after SuccessfulMatchData.new(match_data_description(actual)) + elsif before == after + FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label}", + before: before_inspect, + after: after_inspect + ) else FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label} to #{expected_after.inspect}", - before: before.inspect, - after: after.inspect, + before: before_inspect, + after: after_inspect, expected: expected_after.inspect ) end else FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected_before.inspect}", expected: expected_before.inspect, - actual: before.inspect, + actual: before_inspect, ) end end @@ -54,12 +60,18 @@ module Spectator::Matchers # Performs the test against the expression, but inverted. # A successful match with `#match` should normally fail for this method, and vice-versa. def negated_match(actual : Expression(T)) : MatchData forall T - before, after = change(actual) + before = expression.value + before_inspect = before.inspect + if expected_before == before + actual.value # Trigger block that might cause a change. + after = expression.value + after_inspect = after.inspect + if expected_after == after FailedMatchData.new(match_data_description(actual), "#{actual.label} changed #{expression.label} from #{expected_before.inspect} to #{expected_after.inspect}", - before: before.inspect, - after: after.inspect + before: before_inspect, + after: after_inspect ) else SuccessfulMatchData.new(match_data_description(actual)) @@ -67,18 +79,9 @@ module Spectator::Matchers else FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected_before.inspect}", expected: expected_before.inspect, - actual: before.inspect, + actual: before_inspect, ) end end - - # Performs the change and reports the before and after values. - private def change(actual) - before = expression.value # Retrieve the expression's initial value. - actual.value # Invoke action that might change the expression's value. - after = expression.value # Retrieve the expression's value again. - - {before, after} - end end end diff --git a/src/spectator/matchers/change_from_matcher.cr b/src/spectator/matchers/change_from_matcher.cr index e684e00..0ae3d13 100644 --- a/src/spectator/matchers/change_from_matcher.cr +++ b/src/spectator/matchers/change_from_matcher.cr @@ -25,16 +25,24 @@ module Spectator::Matchers # Actually performs the test against the expression. def match(actual : Expression(T)) : MatchData forall T - before, after = change(actual) + before = expression.value + before_inspect = before.inspect + if expected != before - FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected}", + return FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected}", expected: expected.inspect, - actual: before.inspect, + actual: before_inspect, ) - elsif before == after + end + + actual.value # Trigger block that might change the expression. + after = expression.value + after_inspect = after.inspect + + if expected == after FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label} from #{expected}", - before: before.inspect, - after: after.inspect, + before: before_inspect, + after: after_inspect, expected: "Not #{expected.inspect}" ) else @@ -45,18 +53,26 @@ module Spectator::Matchers # Performs the test against the expression, but inverted. # A successful match with `#match` should normally fail for this method, and vice-versa. def negated_match(actual : Expression(T)) : MatchData forall T - before, after = change(actual) + before = expression.value + before_inspect = before.inspect + if expected != before - FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected}", + return FailedMatchData.new(match_data_description(actual), "#{expression.label} was not initially #{expected}", expected: expected.inspect, - actual: before.inspect + actual: before_inspect ) - elsif before == after + end + + actual.value # Trigger block that might change the expression. + after = expression.value + after_inspect = after.inspect + + if expected == after SuccessfulMatchData.new(match_data_description(actual)) else FailedMatchData.new(match_data_description(actual), "#{actual.label} changed #{expression.label} from #{expected}", - before: before.inspect, - after: after.inspect, + before: before_inspect, + after: after_inspect, expected: expected.inspect ) end @@ -71,14 +87,5 @@ module Spectator::Matchers def by(amount) ChangeExactMatcher.new(@expression, @expected, @expected + value) end - - # Performs the change and reports the before and after values. - private def change(actual) - before = expression.value # Retrieve the expression's initial value. - actual.value # Invoke action that might change the expression's value. - after = expression.value # Retrieve the expression's value again. - - {before, after} - end end end diff --git a/src/spectator/matchers/change_to_matcher.cr b/src/spectator/matchers/change_to_matcher.cr index 57deff5..884b4b0 100644 --- a/src/spectator/matchers/change_to_matcher.cr +++ b/src/spectator/matchers/change_to_matcher.cr @@ -25,19 +25,26 @@ module Spectator::Matchers # Actually performs the test against the expression. def match(actual : Expression(T)) : MatchData forall T - before, after = change(actual) - if before == after - FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label}", - before: before.inspect, - after: after.inspect, - expected: expected.inspect + before = expression.value + before_inspect = before.inspect + + if expected == before + return FailedMatchData.new(match_data_description(actual), "#{expression.label} was already #{expected}", + before: before_inspect, + expected: "Not #{expected.inspect}" ) - elsif expected == after + end + + actual.value # Trigger block that could change the expression. + after = expression.value + after_inspect = after.inspect + + if expected == after SuccessfulMatchData.new(match_data_description(actual)) else FailedMatchData.new(match_data_description(actual), "#{actual.label} did not change #{expression.label} to #{expected}", - before: before.inspect, - after: after.inspect, + before: before_inspect, + after: after_inspect, expected: expected.inspect ) end @@ -65,14 +72,5 @@ module Spectator::Matchers def by(amount) ChangeExactMatcher.new(@expression, @expected - amount, @expected) end - - # Performs the change and reports the before and after values. - private def change(actual) - before = expression.value # Retrieve the expression's initial value. - actual.value # Invoke action that might change the expression's value. - after = expression.value # Retrieve the expression's value again. - - {before, after} - end end end