RSpec matchers for Reform validations with dry-v

[george-carlin]

There's already a 2-year old open issue about this, but it seems to have been abandoned, so I thought I'd get the discussion going again here. Apologies if opening a new issue isn't the right move.

There's a gaping hole in the TRB ecosystem for some clear, reusable RSpec matchers that we can use to easily test that a Reform form has the right validations, similar to shoulda-matchers for activemodel. (Some matchers for other parts TRB like operations might be handy too, but for the purposes of this discussion let's limit it to Reform.)

As discussed on #156, shoulda-matchers won't work out of the box for Reform because it calls validate multiple times on the same form, which Reform doesn't support (by deliberate choice.) Plus shoulda-matchers is for activemodel validations, and all the cool kids are using dry-validation these days.

I've gradually been adding custom matchers to my own project to test reform validations, and I want to open-source them and release them as a gem, or possibly submit them as a PR for an existing TRB gem if you think there's an appropriate home for them already. But before I sink more time into this, I want to get some feedback on what the API should be.

Here's my first attempt:

describe BlogPost::Form do # a subclass of Reform::Form
  include Reform::Matchers::Dry

  # required(:foo).filled
  it { is_expected.to validate_required_key(:foo).must_be_filled }

  # required(:foo).maybe
  it { is_expected.to validate_required_key(:foo).may_be_filled }

  # optional(:foo).filled
  it { is_expected.to validate_optional_key(:foo).must_be_filled }

  # optional(:foo).maybe
  # you can guess what this one will look like :p

  # some other ideas
  it { is_expected.to validate_required_key(:foo).is_gteq(3).is_lteq(10).has_type(Integer) }
  it { is_expected.to validate_required_key(:foo).is_empty }
  it { is_expected.to validate_required_key(:foo).has_max_size(3) }
  it { is_expected.to validate_required_key(:foo).has_max_size(3) }
  it { is_expected.to validate_required_key(:foo).has_size_in_range(4..10) }
  it { is_expected.to validate_required_key(:foo).is_included_in(%[a b c d e f]) }
end

That's with dry-validation in mind. Anything for Reform + ActiveModel will, I think, have to be in completely separate matchers.

Note that these matchers will be testing the behaviour of the form directly (calling validate on an instance of the form) rather than doing reflections on the dry-v schema.

In terms of their external appearance, the big difference between this and shoulda-matchers is that shoulda-matchers has a different matcher for each validation (validate_presence_of,validate_length_of, etc) while here I only have two matchers (validate_required_key and validate_optional_key) which take a bunch of options. This could result in some matchers that are more complicated internally, but IMO this API makes more sense since it's a closer mirror to how dry-v validations are actually declared.

Things also get tricky when you have more complex validations, e.g. how can we write a custom matcher that distinguishes between these two cases?

required(:foo).filled { int? & gteq?(5) }
required(:foo).filled { int? > gteq?(5) }

But for now I think we can start with some matchers that only cover the basic cases, and people can continue to write custom specs for more complex validations if needs be.

What do you think?

[apotonick]

WOW!!!!!!!! :heart_eyes:

[george-carlin]

So you like the API? :)

[apotonick]

Will have to give it a thought or two, but what I love is that someone finally comes up with working examples and suggests something here, instead of waiting for the maintainer to magically figure out what is needed in a real-life scenario. :heart:

[fran-worley]

I like this a lot (though 'maybe' should be all one word). For simple validations these one liners look great I'm just not sure what if anything could be done for more complex cases (i.e. High level rules, validation blocks and predicate blocks, deeply nested schemas/ collections).

With more complex validations that rely on other values, or compose rules using negation or other operators there is a risk that you'll get false positives when testing just one value in isolation. Given that one of the reasons people make the move to using dry-validation is the ability to write complex validations in a cleaner way are we at risk of writing a matchers library that is too restrictive and is virtually unusable for most users of the Reform/dry-v combo?

Just playing devils advocate here as I sit on a train watching the world go by!! 🤓🚂

[apotonick]

I would love higher-level validation matchers, too, so I don't need to specify all of them line by line!!!!

[fran-worley]

Thinking about actual implementation I think you'd need to actually check the result AST from dry-validation to see what predicates failed. We can't just check that an attribute has errors (that wouldn't tell us what caused the error) and we can't rely on the message content as that would break as soon as you used a custom message.

We might be able to clear up simple high level rules (ie a value being dependent on another value) with a WHEN method. I.e

to validate_required_key(:attr).is_empty.when(hash_of_dependent_param_values)

WHEN could also accept a block to determine dependents I.e if we rely on current user being admin or something...

[george-carlin]

"may be" as I've used it above is two words, as in "this value may be filled". ;)

You're right that checking the result AST is a better idea than checking the error messages, I didn't think of that. (shoulda-matchers checks the error message text, and if you use a custom error message you have to pass this into the matcher with an option called with_message or something like that... which is far from ideal.)

To nail down the basics, are we agreed that validate_required_key and have_optional_key are the way to go, or should we think about splitting them into multiple matchers?

Maybe extra matchers can be introduced for higher-level/more complicated rules? Maybe a matcher just called validate? I need to think about how a matcher would handle predicates for the same key that are 'chained' in different ways (e.g. using > vs &.) I suspect this is one of those cases where I won't have a good idea how to do this until I've already written some more code and got a better feel for how everything fits together.

[george-carlin]

(I'd add that may_be_filled isn't the clearest name anyway; doesnt_have_to_be_filled is clearer but that's way too wordy. Other ideas: can_be_blank, can_be_nil, can_be_unfilled?)

[pabloh]

@georgemillo, did you released the gem?. I also found this other one dry-validation-matchers but is no very comprehensive yet and is not using the reflection approach either.