Add a more robust validation error message matcher for models

[cupakromer]

Per discussions in #1026. Consider adding a matcher with the following general format:

expect(record).to have_validation_error("foo bar").on(:attr)

I'm open to discussing alternative names and fluent interface syntax.

In general, this should also have helpful error messages if the model was valid, instead of simply reporting that the error was not found. Additionally, if the error is not found, we should report those errors that created. On the latter, I'm not sure yet if I'd prefer to only see those other errors on the same attribute (it could possibly be none), or all errors for the model (which could potentially be a lot).

[myronmarston]

Shouldn't that be record instead of collection_owner?

[cupakromer]

Probably, I was just copy-pasting what was in the old error_on messages.

[JonRowe]

So compared to the old way of doing this, this is better, but I worry this will cause more people to cargo cult validation testing (which I think is a smell, testing code you don't own)...

[cupakromer]

@JonRowe Not sure I follow. This is more of the: 1) setup state, 2) call method, 3) assert side effect. There's nothing really magical about this as it tests the full model stack.

I'm open to alternative ideas / implementations.

[JonRowe]

There's nothing really magical about this as it tests the full model stack.

Which you don't own, so in my opinion it's a bad practise to test in this fashion. You're better off testing the business need for that validation rather than the specific implementation which could change on Rails whim and you won't be able to do anything about it.

Things that make bad practises easier should be avoided ;)

[cupakromer]

Not sure I agree completely. I'm testing my expectations on a model I own; the fact it is based on AR is an implementation detail (I easily could base it on something else) which doesn't affect the matcher at all. I'm not directly mocking/stubbing/messing with AR under the hood. These specs make sure my expectations with how I interact with my model are correct:

when I have an XYZ invalid thing, after asking it if it is valid; I expect to see it have ABC error message

I'd rather it fail here instead of more mysteriously higher up in an integration / acceptance spec.

[JonRowe]

I'm testing my expectations on a model I own; the fact it is based on AR is an implementation detail (I easily could base it on something else) which doesn't affect the matcher at all.

It does, the matcher is tied explicitly to the AR implementation detail, you don't own that code, if Rails decided to change how that worked this would have to cope with that and then special case across versions etc.

I'd rather it fail here instead of more mysteriously higher up in an integration / acceptance spec.

Except that is where it's truely decoupled from your implementation, you then don't care how that's implemented either via ActiveRecord validation, or a database constraint, or an input validation (think form object).

[cupakromer]

Hmm. As always, I truly value your insight and feedback. 💙 💙

I'm interested to know what you think is explicitly tired to an AR implementation. Is it that calling: valid? updates an accessor errors which acts like a hash?

[JonRowe]

Expecting both valid? and errors_on, and that process. It's hand wavy to say it "could be implemented by something else" realistically this is rspec-rails, it is going to be implemented by ActiveModel, and the most realistic alternate, to use a database constraint, would not meet this matcher. Thus it's tied to code you don't own.

[cupakromer]

Hmm. Maybe a typo, I only expect valid? and errors (which acts like a hash). This could work (and does) for say Mongoid (though that is AR-y) and possibly other DB adapters that match the same protocol.

At the end of the day, it's about does this thing meet the protocol I expect. If not, then the object under test (in this case a model instance / record) doesn't meet my current expectations. Whether that's a custom matcher or part of rspec-rails proper I think is what we may be discussing.

Can I see this being an issue to maintain in the future if AR changes it's protocol? Yes. However, we already have precedence for it in rspec-activemodel-mocks and rspec-collection_matchers. Though, we have pulled both of those out into separate gems. Maybe we should consider an rspec-activemodel_matchers gem?!?!

[bbugh]

I agree with @JonRowe in the standard Rails cases with basic validations, however, custom validations are rather common and testing them explicitly on the unit level (in addition to the integration level) has been the right approach to full coverage in my experience.

For example, we have a model that has different states, and some states have different conditional validations. This model has complex dynamics with standard ruby objects, it does not have a resourceful controller, and it will largely only be used by resque and not by users. While it is important to test on the integration level, it has been very beneficial to cover the unit behaviors. There have been a number of times where the unit test has caught a bug during refactoring that cascaded down into multiple integration specs that would have been much harder to track down if we hadn't had precise identification of the custom validation state causing the trouble. While this is a complex case, I have not found this to be that uncommon in large Rails projects.

If have(n).errors_on(:attr) is not "respectable" behavior (as that seems to be what prompted the extraction of rspec-collection_matchers), I think the world would benefit from a recommended best practice (as @cupakromer does not seem to like his own suggestion 😄).

I am also happy to submit a pull request for the desired behavior described in this issue. My personal preference of syntax would be something like:

# check for any validation error
expect(record).to have_validation_error_on(:attr)
# or with optional message match
expect(record).to have_validation_error_on(:attr).with_message('error')
# or
expect(record).to have_validation_error_on(:attr).matching(/error/)
expect(record).to have_validation_error_on(:attr).matching("can't be blank")

[cupakromer]

I agree this isn't something to put into rspec-rails. There some tentative plans at my current employer to release an open source gem with alternative validation matchers.