Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Differ tests no longer use Differ to report diff expectation #174

Merged
merged 1 commit into from
Feb 8, 2015

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Feb 8, 2015

For example, previously we would write our differ_spec expectations as

 actual = differ.diff(str1, str2)
 expect(actual).to eq(expected)

Which had the suprising (but expected) result
of failure diffing changing the string

 actual = differ.diff(str1, str2)
 actual != expected &&
   RSpec::Expectations.fail_with(message, actual, expected)

RSpec::Expectations.fail_with calls differ.diff(actual, expected)
where differ is

**`RSpec::Support::Differ.new**(
   :object_preparer => lambda { |object|
RSpec::Matchers::Composable.surface_descriptions_in(object) },
   :color => RSpec::Matchers.configuration.color?
  )

So, all these differ_spec.rb tests where we say expect(str1).to eq(str2) will
run through the differ twice on failure.

Rather than test them as expect(str1 == str2).to eq(true), use the #be_correctly_diffed matcher.

We're not testing encoding because strings created in differ_spec.rb have a
default encoding of UTF-8 in all cases, but string created in the differ.rb
may be US-ASCII in MRI prior to 2.0 else UTF-8.
https://github.com/ruby/ruby/blob/aacc35e144/encoding.c#L1741

Refs:

@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2015

Ok I like this approach so this isn't a block, but I'm wondering if "correctly" diffed is the right name for the matcher, be_diffed_as(expected) seems more natural.

For example, previously we would write our differ_spec expectations as
  actual = differ.diff(str1, str2)
  expect(actual).to eq(expected)
Which had the suprising (but expected) result of failure diffing changing the string

  actual = differ.diff(str1, str2)
  actual != expected &&
    RSpec::Expectations.fail_with(message, actual, expected)

RSpec::Expectations.fail_with calls differ.diff(actual, expected)
  where differ is **RSpec::Support::Differ.new**(
    :object_preparer => lambda { |object| RSpec::Matchers::Composable.surface_descriptions_in(object) },
    :color => RSpec::Matchers.configuration.color?
   )

So, all these differ_spec.rb tests where we say expect(str1).to eq(str2)
will run through the differ twice on failure.

Rather than test them as expect(str1 == str2).to eq(true), use
the #be_correctly_diffed matcher.

We're not testing encoding because strings created in differ_spec.rb have a default
encoding of UTF-8 in all cases, but string created in the differ.rb may be
US-ASCII in MRI prior to 2.0 else UTF-8.
https://github.com/ruby/ruby/blob/aacc35e144/encoding.c#L1741
@bf4 bf4 force-pushed the dont_diff_differ_failures branch from f0f25f0 to 7700d2e Compare February 8, 2015 15:27
@bf4
Copy link
Contributor Author

bf4 commented Feb 8, 2015

@JonRowe renamed. There is duplication with the matcher in encoded_string_spec but I figure that can be a later refactor if desired.

JonRowe added a commit that referenced this pull request Feb 8, 2015
Differ tests no longer use Differ to report diff expectation
@JonRowe JonRowe merged commit 95bfe85 into rspec:master Feb 8, 2015
@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2015

Thanks, would you like to tackle the duplication?

@bf4 bf4 deleted the dont_diff_differ_failures branch February 8, 2015 22:26
@bf4
Copy link
Contributor Author

bf4 commented Feb 8, 2015

Sure. What do you think about putting it in lib/rspec/support/spec/string_matcher.rb and requiring it directly in the two specs?

@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2015

Sure

@bf4
Copy link
Contributor Author

bf4 commented Feb 9, 2015

ref #176

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants