reform matchers

[musaffa]

I'm planning to port shoulda matchers to reform. Should it to be integrated into reform or you want a separate gem like reform-matchers?

[apotonick]

Now this is funny, because I talked about matchers with @fernandes today.

I personally don't understand what these helpers are good for? Are they meant to test "basic" validators, only? I'm a bit confused, because using matchers doesn't test my form behaviour but only its configuration. Doesn't it make more sense to test something along

form.validate({})
form.errors.to_s.must_equal ".."

In this test, I know that my form works while with matchers I can't be sure that this is the case. It becomes even more complicated when you have more complex validators that might interfere with each other - how do you test that with matchers? Or do you have to write real tests, then?

I totally can see how they help to assert "simple" validations without a million lines of test code, but ..... how do you integrate that with real tests? Please, enlighten me, guys!

[musaffa]

The most parts of a model are declarative in nature. I use shoulda matchers to test those declarations and i write normal tests for other parts. Since a form object is mostly declarative, it makes sense to have matchers to test those declarations.

Matchers can be real unit tests. For example, presence validator matcher sends empty data for an attribute and expect it to be invalid. It will also be possible to make it expect for an error massage (if you really want them).

[apotonick]

It would be great if those matchers were real unit tests - what do you think?

[musaffa]

I think shoulda matchers are already real unit tests. For example, when it tests for a validation, it sends valid and invalid data internally to check the validity of the model object. Shoulda matchers also support what message to expect if a validation fails. Just have a look at the source of ensure_length_of_matcher.

[fernandes]

@apotonick today I checked the should-matchers deeper than yesterday and yes, it runs validations

I think in this case we need to think should-matchers port not as a silver bullet, it will make tests easier for validates presence, length, numerically and basic stuff... for some complex cases, we need to write our own, like we do in pure AR

@musaffa great initiative, IMHO this "port" can help very much, if we are DRYing forms, views, business logic, why not tests? :+1:

If you wanna, do the kickstart and I help as I can

[apotonick]

Ok, if these matchers/macros do actual unit tests I'm delighted to use them myself in my code! I thought they were just doing some reflections on the model/form.

[musaffa]

@fernandes I will let you know. thanks! @apotonick my initial question is still left unanswered.

[apotonick]

Whoops, sorry @musaffa! :smile: I vote for reform-matchers so you can be King of the Castle! Thanks so much buddy.

How are you gonna implement the matchers? Real unit tests that call form.validate(..) and form.errors?

[musaffa]

I will try. :)

[musaffa]

In case you are interested, I have made a Polymorphic Constraints gem which enforces database constraints on polymorphic relations using triggers. It supports sqlite, postgresql and mysql.

Any suggestion will be highly welcome.

[apotonick]

That is pretty cool, man! Any chance we could port this on a higher level, so we can use those constraints in the form? I don't want the DB to know any business logic in my architecture. What's your thoughts on this?

Anyway, the gem looks great, I'm impressed!!! :+1:

[musaffa]

It would be pretty straight forward to implement this in the application code but data integrity cannot be ensured. I wanted to mimic foreign key constraint for polymorphic relations so that data integrity is maintained at the database level.

[fantgeass]

@musaffa what is the status of your gem? If you already started, I can help with development.

[musaffa]

@fantgeass I've been busy with another project. I'll start soon. :)

[zavan]

I've been using shoulda matchers with trailblazer/reform/rspec like this:

  describe "the contract" do
    subject { Product::Create.present(params).contract }

    it { is_expected.to validate_presence_of :name }
    it { is_expected.to validate_presence_of :weight }
    it { is_expected.to validate_presence_of :width }
    it { is_expected.to validate_presence_of :height }
    it { is_expected.to validate_presence_of :length }

    it { is_expected.to validate_numericality_of(:weight).is_greater_than(0) }
    it { is_expected.to validate_numericality_of(:width).is_greater_than(0)  }
    it { is_expected.to validate_numericality_of(:height).is_greater_than(0) }
    it { is_expected.to validate_numericality_of(:length).is_greater_than(0) }
  end

Works fine.

[musaffa]

so it works out of the box. cool!

[zavan]

@musaffa Yeah, it seems to, but I haven't stumbled in any edge cases yet.

Right now I'm building a form with a nested collection, something like:

class OrderForm < Reform::Form
  property :number,      validates: {presence: true}
  property :customer_id, validates: {presence: true}

  collection :line_items do
    property :product_id, validates: {presence: true}
    property :quantity,   validates: {numericality: {greater_than: 0}}
  end
end

I'm still not there so I haven't yet thought about how am I going to test those nested properties validations from the collection, maybe I should extract them into another form and just include them there, or maybe that'll be a nice excuse for the reform-matchers creation :smile:

[musaffa]

LOL yes it is.

[fernandes]

@zavan is this way of using shoulda matchers is still working with reform 2.0 and latest trailblazer?

I'm getting a wrong number of arguments (0 for 1) on reform-1.2.6/lib/reform/contract/setup.rb:4

ps: I'm using Reform::Form.reform_2_0! on initializer

thank you

[fernandes]

as I've checked:

shoulda uniqueness matcher instantiates a new model using Model.new and setup needs the model as parameter

ps: I tested with presence matcher and worked fine

FYI, for uniqueness I've made my own test:

context "with existing domain"do
  it "error existing domain" do
    params = attributes_for(:dummy)

    Dummy::Create[params].model
    dummy = Dummy::Create.run(params).last
    expect(dummy.contract.errors[:domain]).to include('has already been taken')
  end
end

if someone has a better idea on make this, I'm all ears :wink: thank you!

[apotonick]

I do uniqueness tests like that, too.

We should provide our own matchers in addition to what exists! Is there any way we could use the matchers from above plus our own uniqueness, etc? In Rspec AND MiniTest (I don't use Rspec)?

[apotonick]

The more tests I write with forms the more redundancy I can see, so I'm really all in for Reform/Operation matchers. Not that redundancy in tests is wrong, but simple matchers will save time and brain power when writing tests.

We have the advantage that we have data schema and validations cleanly separated from models in the operation, so it should be a nice task to implement matchers.

[shingara]

the format

describe "the contract" do
    subject { Product::Create.present(params).contract }

    it { is_expected.to validate_presence_of :name }
end

not work anymore since 2.0.3 because valid? become private

[karlingen]

@shingara Yup, not working anymore. valid? is private and breaks stuff.

Failure/Error: it{ should validate_presence_of :text }
     NoMethodError:
       private method `valid?' called for #<Barbell::News::Create:0x007f9b6ad40668>

[karlingen]

I managed to workaround this by adding the following to my form:

def valid?
  super
end

Is there a good reason why this method is private? @apotonick

[apotonick]

Yepp! :grin: The form's API is result = validate(params) and that's the only single entry point to invoke that logic. I want to reduce state in the form and flatten the public API, which is why we won't have valid? at all in future versions.

Just because ActiveRecord allows this doesn't mean it's the right thing to do. The opposite is the case: people have to understand that there's only one API method to run a validation and retrieve the result.

Using isolated matchers is wrong, in my opinion. A form is a collection of validations that might have side-effects, testing them step-wise in isolation contradicts testing them at all, because you're not testing a production scenario (it will never happen that you have a form submission with one field being submitted, only).

I agree that there should be testing helpers to quickly test formal standard validations, but matchers the way you use it are not the right way. The form has to be fully submitted via validate. Again, this is something Rails implemented and now everyone thinks this is the way to go.

Imagine you had custom validations that rely on each other. Side-effects in your artificial must_validate_presence matcher will never catch those. Can you show me some examples of how you test forms presently?

In Trailblazer, and this is where all my gems are heading, you don't test forms as a stand-alone entity anymore but the entire operation, which assures all side-effects are covered.

Sorry if that doesn't help you but the same discussion is somewhere in these issues from around 6 months ago... :laughing:

[karlingen]

Thanks for the clarification!

I'm writing a gem called deadlift that uses Contracts from your gem and calls them Barbells so they can be used by the deadlift objects.

This is how I test a barbell (contract):

# app/business_logic/deadlifts/news/barbells/create.rb
module Deadlifts
  module News
    module Barbells
      class Create < Deadlift::Barbell::Base
        model :news

        property :title
        property :text

        property :location
        property :current_user

        validates :title, presence: true
        validates :text, presence: true
      end
    end
  end
end

# spec/business_logic/deadlifts/news/barbells/create_spec.rb
require 'spec_helper'

describe Deadlifts::News::Barbells::Create, type: :model do
  let(:params){{}}
  subject{described_class.new(params)}

  it{ should have_model_name :news }

  it{ should have_property :location }
  it{ should have_property :current_user }

  it{ should have_property :text }
  it{ should have_property :title }

  it{ should validate_presence_of :title }
  it{ should validate_presence_of :text }
end

And these are my custom matchers:

# spec/support/matchers/have_property.rb
RSpec::Matchers.define :have_property do |expected|
  match do |actual|
    if actual.respond_to?(expected) && actual.respond_to?("#{expected}=")
      true
    else
      false
    end
  end

  failure_message_when_negated do |actual|
    "expected that #{actual} did not have a property named #{expected}"
  end

  failure_message do |actual|
    "expected that #{actual} had a property named #{expected}"
  end

  description do
    "have the property #{expected}"
  end
end

RSpec::Matchers.define :have_model_name do |expected|
  match do |actual|
    actual.class.model_name.to_s == expected.to_s.camelize
  end

  failure_message do |actual|
    "expected that #{actual} had a model name #{expected}"
  end

  failure_message_when_negated do |actual|
    "expected that #{actual} did not model name #{expected}"
  end

  description do
    "have the model name #{expected}"
  end
end

[apotonick]

Nice stuff! I just checkout out your gem, and it's very similar to Trailblazer::Operation! :grimacing:

[karlingen]

Wow, I wish that I had found out about Trailblazer earlier! Removing my gem now.. 😂

[valscion]

I just chatted with @apotonick, and he's very open to creating a new gem for the Reform matchers.

[samstickland]

Hi,

I just stumbled across this issue whilst looking for information around using shoulda-matchers and reform. I am having issues with some of the shoulda matchers (such as the length matches) that set attributes and call valid? multiple times within a single test. Reform doesn't clear the errors between valid?/validate so the matchers fail. See: https://github.com/apotonick/reform/issues/368

Even if we create a separate reform-matchers gem then I think it will still have the same problem, since wanting to try multiple value permutations in a single test is kinda necessary for length matches.

@apotonick Regarding validators that rely on each other, I usually do something like this:

       subject(:form) { Artist::Create.contract_class.new(Artist.new) }

        it { is_expected.to validate_presence_of(:name) }

        context "when platinum" do
          before { form.platinum = true }

          it { is_expected.to validate_presence_of(:legal_type) }
        end

EDIT: Actually, I know valid? is going away, but thinking about it I'm not sure that setting attributes directly on forms is supported either?

In which case, would an interface like this be better?

it { is_expected.to validate_presence_of(:legal_type).with_params(platinum: true) }
it { is_expected.to_not validate_presence_of(:legal_type).with_params(platinum: false) }

[samstickland]

Right, I've added a PR (https://github.com/thoughtbot/shoulda-matchers/pull/934) to shoulda-matchers so that it calls errors.clear before valid? so that the various matchers that call valid? multiple times now work correctly with reform. The fix is also available here: https://github.com/samstickland/shoulda-matchers/tree/3.1.1_with_clear_errors

If valid? is going away then I'll need to make more changes to shoulda-matchers.

@apotonick Is the following style of setting attributes and validating going to stay around?

form = Artist::Update::.new(Artist.new)
form.name = 'x' * 201
form.validate({})

If so, I don't think the shoulda changes should be too extensive.

[apotonick]

Shoulda matchers should not use any private API from Reform. They must use validate to set values and retrieve the result and must re-instantiate the Reform object for every "test". This is how Reform is designed, anything else (especially things like errors.clear or valid?) will break at some point, as we're not maintaining the old, out-dated ActiveModel API.

It is super important to communicate that to the shoulda team, and it's probably a good idea to start something such as shoulda-reform.

[samstickland]

OK, in that case there's no way that the ActiveModel shoulda-matchers can be made in any shape or form to work with Reform + ActiveModel validations.

The length matchers call valid? multiple times to test with various string lengths. Even a simple maximum length test will check with a string that's over the maximum length and one at the maximum length.

I understand that you won't be maintaining the ActiveModel Validations in Reform, but if someone was using them, would errors.clear still be a part of that?

[george-carlin]

Just wondering if there's any consensus on this and if there's any general agreement on the best (or merely a better) way to test validations on a Reform form?

[apotonick]

Matchers would be awesome, but they have to run the real validation and check the result. It's funny, I was just working on trailblazer-rspec, so we could start with this right now, @georgemillo !

[george-carlin]

So you're suggesting adding shoulda-like validation matchers to trailblazer/rspec-trailblazer? That sounds good. I'd suggest splitting them out into a separate gem called rspec-reform for those of use that are using reform without trailblazer.

But come to think of it, we might need different matchers and/or a slightly different DSL for forms that use ActiveModel::Validations vs forms that use dry-validation. Maybe the real solution is to create matchers that are designed specifically for for testing dry validations, and try to keep them agnostic as to whether the model is a Reform::Form or some other object that's using dryv? (I.e. create a more generic gem dry-validation-matchers instead of a gem called reform-matchers that's tightly coupled to reform.)

What do you think?