Fixup issue with multiline diffs in custom matchers. #423

Merged
merged 2 commits into from Feb 6, 2014

Projects

None yet

2 participants

Owner
JonRowe commented Jan 12, 2014

Fix #420 by not forcing expected to be an array in custom matchers.

Owner
JonRowe commented Jan 12, 2014

I''d like a review on this to make sure its a sensible strategy for solving this problem. /cc @myronmarston

Owner

Yep, planning to review this tonight. I'm out most of the afternoon.

@myronmarston myronmarston and 1 other commented on an outdated diff Jan 13, 2014
lib/rspec/matchers/dsl.rb
@@ -287,7 +287,13 @@ class Matcher
def initialize(name, declarations, *expected)
@name = name
@actual = nil
- @expected = expected
+ if expected.size == 1
+ @no_array = true
+ @expected = expected[0]
+ else
+ @no_array = false
myronmarston
myronmarston Jan 13, 2014 Owner

What is @no_array meant to be used for? I see it set here but never used again.

JonRowe
JonRowe Jan 14, 2014 Owner

Was for something that later proved unnecessary...

@myronmarston myronmarston commented on the diff Jan 13, 2014
spec/rspec/matchers/dsl_spec.rb
@@ -322,7 +322,12 @@ def foo
it "provides expected" do
matcher = new_matcher(:name, "expected string") { }
- expect(matcher.expected).to eq ['expected string']
+ expect(matcher.expected).to eq 'expected string'
myronmarston
myronmarston Jan 13, 2014 Owner

As you've shown here, this is a breaking change, so if we go this route, we'll need to provide a deprecation warning on 2.99 when #expected is called and it's an array of 1 element.

JonRowe
JonRowe Jan 14, 2014 Owner

None of the other matchers behave like this, but I guess you're right it could affect people reliant on this quirky behaviour, I'll add a 2.99 deprecation warning after this is merged.

Owner

This looks like a reasonable fix, although it doesn't demonstrate adequately that it fixes the diff issue since there's no spec here demonstrating that the diff is the same for a custom matcher as for a matcher like eq. What do you think of bringing over the spec I wrote up in #420? That would help demonstrate this fixes the bug and also guard against future regressions since the diffing behavior of custom matchers is an important behavior to maintain in future releases.

Owner
JonRowe commented Jan 14, 2014

Added a spec, it's not quite as simple as proposed in #420 cause ASCII control codes...

Owner

LGTM.

Owner
JonRowe commented Jan 14, 2014

I'd like to hold off till I get a chance to look at the behaviour of expected/actual a bit more actually, I got a weird diff with true/false when playing around with the sample custom matcher.

Owner

What's an example of where this produces a weird diff?

Owner
JonRowe commented Jan 24, 2014

Did you get a chance to look at the weird diff?

Owner

Not yet, sorry, thanks for the reminder.

Owner
JonRowe commented Feb 6, 2014

Ping?

Owner

So, looking at your example, I think the diff from before is weird, and the new behavior with this change (e.g. no diff) is correct. What do you think is weird about it?

Owner
JonRowe commented Feb 6, 2014

I'm ok with the lack of diff on the true/false but note that the strings aren't being diffed either...

Owner

I'm ok with the lack of diff on the true/false but note that the strings aren't being diffed either...

We only provide diffs when there are multiline strings involved:

https://github.com/rspec/rspec-expectations/blob/9003d2d7ef0c124f06f135a436d84c031947a608/lib/rspec/expectations/fail_with.rb#L24-L26

Your example did not provide multiline strings so I would not expect a diff.

Owner
JonRowe commented Feb 6, 2014

Fair enough

Owner
JonRowe commented Feb 6, 2014

Merging because the rbx build issue is separate.

@JonRowe JonRowe merged commit 0a961b5 into master Feb 6, 2014

1 check failed

default The Travis CI build failed
Details
@JonRowe JonRowe deleted the fixup_issue_with_multiline_diffs_in_custom_matchers branch Feb 6, 2014
Owner

You still planning to add a deprecation warning to 2.99 for this?

Owner
JonRowe commented Feb 6, 2014

Yeah on my list

@JonRowe JonRowe restored the fixup_issue_with_multiline_diffs_in_custom_matchers branch Feb 9, 2014
@JonRowe JonRowe deleted the fixup_issue_with_multiline_diffs_in_custom_matchers branch Feb 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment