Fail Include matcher when comparing String with non-String

[naveg]

If the actual is a String and the expected is not, a TypeError occurs
when we try to call actual.include?, becuase this method expects
a String when called on a String.

If the actual is a String and the expected is not, then the match
can't possibly succeed and we can return early.

[naveg]

@pirj the build appears to be failing because tests that are part of rspec-rails are documenting the error that this PR fixes

we probably also want rspec/rspec-rails#2728, but I'm not sure how to best sync up the two changes

[pirj]

Thank you!
Let's safeguard rspec-rails's suite from failures, and this is good to merge.

[JonRowe]

Thanks for taking the time to submit this but I'm leaning towards closing this, Ruby already fails this test for us with a helpful type error, so actually this removes information from the failure, additionally if someone were to monkey patch or refine String to allow include? to match against other things this would break that...

(Requesting changes to block merging here until I'm convinced)

[naveg]

@JonRowe just invert the matcher to see why this is a bug in rspec:

expect("foo").not_to include({:bar => "baz"})

I think this example should pass, but it currently explodes with an error.

[JonRowe]

I'd actually expect an error, you're matching two incompatible objects its actually a false positive for that to pass... imagine a scenario where you're gating on using the include? call, you write a test to assert that some input would cause your code to reject it and your example passes, but in production it would raise

[JonRowe]

Thank you for the example, its actually pushed me over the fence; I'm going to close this because the false positive of this passing a negation is too much of a behaviour change, your example should fail because a production usage of that would cause an exception.

[naveg]

@JonRowe I disagree, but it's your call. We'll just have to work around this behavior in our tests.

Ruby is dynamically typed, and checking for inclusion in a test does not imply that we test for inclusion in production code. We are simply asserting something about the shape of the value.

Here's another example that also explodes with the same error:

expect(["foo", {:bar => "baz"}]).to include(a_hash_including({:bar => "baz"}))

[JonRowe]

I respect your different opinion but I have to think of everyones usage and in particular the potential for the harm a false positive causes vs having to catch the error explicitly, I can appreciate its annoying to have to assert on an error vs to_not include but its explicit vs silent; In general we don't prevent Ruby errors from happening because we want to respect Ruby idioms.

Once again I thank you for taking the time to contribute and discuss this and I'm sorry we can't include this on this occasion.