Make MismatchError a base Exception

[iamvery]

Problem

In my experience the scientist gem is very useful with legacy and complicated code bases. I have found success in adding an experiment and levering the test suite along with production to identify mis-placed expectations and adjust. However, in some cases these code bases make the unfortunate decision to use bare rescue => e constructs for reasons. Since scientist's MismatchError inherits from StandardError it will be caught up by such rescues. If I'm not mistaken, the purpose of this error (which is off by default) is to increase the visibility of mismatches, particularly in test suites. If possible, they should avoid being rescued similar to how certain RSpec errors behavior.

Solution

Update MismatchError to inherit from Exception so that it isn't handled by a bare rescue => e. I'll admit there are subtleties here that I probably don't fully appreciate, so I offer this solution as the start of a conversation.

Thank you for the wonderful library! ❤️💙💚💛💜

[coveralls]

Coverage increased (+0.003%) to 99.11% when pulling ad48edc on iamvery:iamvery/errors into 1105e41 on github:master.

[rick]

Oh, interesting ... Yeah ,I'm not opposed to this idea, and the current suite passes. Could you add a test that exercises this path (i.e., wraps an experiment in rescue => e with raise_on_mismatches set to true and ensure that the exception bubbles up properly?

[iamvery]

@rick Great idea! I will try to get to that 🔜

[iamvery]

@rick added an example. Let me know what you think

[rick]

I like the test. @zerowidth wondering about your thoughts on this. The case that I could see as problematic is if someone's running experiments are relying on being able to catch MismatchError via rescue (of StandardError) in enclosing code. Rolling out this change could result in breakage in systems that do that.

[iamvery]

Good call, Rick. I would also be concerned for folks in this position as it might create a significant amount of work for them to fix up their app/test suite.

To do this myself, I used a copy of the MismatchError that inherits from Exception via raise_with and it was non-trivial to fix up the test suite.

[zerowidth]

I can't think of a reason this would be an issue. We don't rescue MismatchError anywhere in our code that I can find, and that's intentional so tests can fail. Changing the error base class should make the raise-on-mismatch behavior even more reliable.

[iamvery]

I can't think of a reason this would be an issue.

I think the concern is that changing this behavior could result in additional errors bubbling up in user's code, resulting in more work for them. In our case, it would have resulted in a red test suite with hundreds of failures. Surprising and frustrating. That said, each of those failures would indicate work that should have been done previously to complete the experiment. Thanks for responding! Looking forward to using this :)