rubocop / rspec-style-guide

Best practices for writing your specs!
http://rspec.rubystyle.guide
953 stars 118 forks source link

Check model validity #119

Open choosen opened 3 years ago

choosen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 : )