The Differ no longer catches CompatibilityErrors

[bf4]

This code was introduced in
https://github.com/rspec/rspec-expectations/pull/220/files#diff-9533f5f156a38a3307ecfc610d2282d7R47

but is no longer raised either by the the current test code, the original test
code
@expected="Tu avec carté {count} itém has".encode('UTF-16LE')
@Actual="Tu avec carte {count} item has".encode('UTF-16LE') or any other
variations I've tried.

So, I'm proposing to remove it, especially since encoding
is well-supported and tested by EncodedString, which the Differ relies on.

[JonRowe]

Sorry I don't want to remove this, I have seen this triggered in issues people have opened and its notoriously hard to replicate on a standard system so I'd rather have this safety net.

[bf4]

Np, just wanted to put it out there for reference. I'd love to test if you see examples.

[JonRowe]

If you'd like to fake up a spec for it to show what it does when triggered I'd merge, but I've had difficulty triggering this on Travis without stubbing out a call.

[bf4]

I spent quite a bit of time trying to get the differ to raise this exception and never succeeded, so, maybe we should change the message to

If you are reading this error message, please report it with backtrace and string to rspec-support 173 :)

[myronmarston]

Sorry I don't want to remove this, I have seen this triggered in issues people have opened and its notoriously hard to replicate on a standard system so I'd rather have this safety net.

I lean the other way -- I really hate leaving code in place "just in case". If we cannot find a way to replicate that particular exception I'd rather not continue to have the baggage of the (apparently) unneeded rescue.

I think the worst case to removing it is some user gets an error, and they report it, which allows us to turn that into an actual regression spec, fix it, and get a patch release out quickly with the fix.

If we do decide to keep that rescue, I think it really belongs in EncodedString, not in the differ...after all, the entire point of EncodedString is to deal with these encodings issues so that other places in RSpec don't have to do rescues like this. Dealing with encodings is entirely outside the responsibility of the differ.

[JonRowe]

I lean the other way -- I really hate leaving code in place "just in case". If we cannot find a way to replicate that particular exception I'd rather not continue to have the baggage of the (apparently) unneeded rescue.

Normally I'd agree with you, but this is an observed edge case, just one we can't replicate on a standard UTF-8 machine, but saying we should remove it because we can't replicate it is like saying we should remove our windows fixes because we can't replicate them on OS X.

If we do decide to keep that rescue, I think it really belongs in EncodedString, not in the differ...after all, the entire point of EncodedString is to deal with these encodings issues so that other places in RSpec don't have to do rescues like this. Dealing with encodings is entirely outside the responsibility of the differ.

I'm ok with moving it...

[myronmarston]

Normally I'd agree with you, but this is an observed edge case, just one we can't replicate on a standard UTF-8 machine, but saying we should remove it because we can't replicate it is like saying we should remove our windows fixes because we can't replicate them on OS X.

Well, if you're sure that the error can still occur that's different. I thought we were at the point where we thought it couldn't but you wanted to leave the rescue in place "just in case". As for the standard UTF-8 bit...I believe we can write specs (potentially that shell out to a new process if necessary) that have different default encodings in order to replicate such things.

I'm ok with moving it...

I'd like to see that. As I said, it feels like an SRP violation and would also likely require us to rescue that error elsewhere if/when we start using EncodedString elsewhere.

[bf4]

We'd likely need to introduce more EncodedString objects into the differ.rb. The compatibility error, is, I think tested against in EncodedString. Also, we can totally avoid it with Encoding.compatible?(a,b)., via #pick_encoding

[bf4]

My 2 💰 is that it's a judgement call which way to go, and either way should to lead to a user report if there's a failure.

[JonRowe]

My concern is that diff-lcs can (and will) generate this error if we provide it with encoding that isn't compatible with what it uses (and it will also have internally encoded strings based on any magic comments they use) which is what I think has triggered this in the past (we've encoded the strings accordingly to the user but thats conflicted with diff-lcs.