Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check model validity #119

Open
choosen opened this issue Jul 24, 2021 · 2 comments
Open

Check model validity #119

choosen opened this issue Jul 24, 2021 · 2 comments

Comments

@choosen
Copy link

choosen commented Jul 24, 2021

Content below exists in guide from the initial version

Add an example ensuring that the model created with FactoryBot.create is valid.

describe Article do
  it 'is valid with valid attributes' do
    expect(article).to be_valid
  end
end

I would not add additional code to spec factory validity if factory bot is delivering that one (also with traits: true)
see: https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#linting-factories

@pirj
Copy link
Member

pirj commented Jul 24, 2021

It is very reasonable to use .lint, especially with traits: true. Very well worth it.

This, however, doesn't work in certain situations, still, workarounds exist.

  1. "Abstract" factories
    Disclaimer: abstract factories don't exist if FactoryBot.
    If there's an abstract model, that is not intended to be instantiated, the factory creation will fail.
factory :user do
  role { nil } # this makes the model invalid

  factory :admin do
    role { :admin } # valid now
  end
end

Solution: exclude :admin from linting by explicitly giving lint a list of factories to lint.
Not so easy when the factory is designed in a way that a trait needs to be used, e.g. replace factory :admin with trait :admin.

  1. Other strategies
    lint only checks :create by default, and does not :build and :stub. With after(:create) callbacks, the spec would tell you that the factory is solid, while your build and build_stubbed models might be actually invalid.

  2. lint is slow
    In a number of projects, .lint spec example was by far the slowest one, taking up minutes even without traits: true. That made the spec suite poorly parallelizable.
    Solution:

  • split into several examples somehow, each linting a group of factories
  • include lint to a specific model spec
    Downsides: due to human error, some factories might be left out.
    It might be an idea for a rubocop-rspec cop to add a lint when a factory is used later in the spec, however, :product factory might be used in user_spec.rb, and for the cop it's impossible to distinguish if it's a user-related factory and it should be linted in the spec or not, and it should be linted in a corresponding model's spec.

Bottom line: would you like to amend the current recommendation, shifting it towards lint?

@choosen
Copy link
Author

choosen commented Jul 24, 2021

Ad. 1
That's quite simple to reject them (see below) and abstract factories can be tested at model spec for validity as edge case.

factories_to_lint = FactoryBot.factories.reject do |factory|
  factory.name =~ /^old_/
end

FactoryBot.lint factories_to_lint

Ad. 2
You can pass strategy :build to .lint - but I did not test it actually

Ad. 3
Guard for linting only changed factories would be great :D (relations could be detected by reflection system)
If lint is so slow, then you need to split your models into 2 rake tasks and run them in parallel.
But nevertheless if lint is slow then checking all in model specs will not be faster.

// So I see that you have fast specs beside validations 👍

Ad. Bottom line
I am not great writer with flair but I will draft something : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants