Reputation: 41
I have email factory (spec/factories/email.rb):
FactoryGirl.define do
factory :email, class: String do
skip_create
transient do
username 'user'
subdomain 'mail'
domain_name 'example.com'
host { [subdomain, domain_name].compact.join('.') }
end
trait(:with_blank_host) { host '' }
trait(:with_blank_username) { username '' }
initialize_with { new("#{username}@#{host}") }
end
end
And I have spec (spec/models/user_spec.rb):
RSpec.describe User, type: :model do
# ...
it { is_expected.to_not allow_value(FactoryGirl.create(:email, :with_blank_host)).for(:email) }
it { is_expected.to_not allow_value(FactoryGirl.create(:email, :with_blank_username)).for(:email) }
end
Is it correct to use FactoryGirl in this way? Is it a bad practices?
Upvotes: 3
Views: 165
Reputation: 3513
Assuming the logic behind creating those email without host/username won't be use anywhere else but the tests. Why do you want to keep it in the factory?
Why not simply doing:
FactoryGirl.define do
factory :email, class: String do
skip_create
transient do
username 'user'
subdomain 'mail'
domain_name 'example.com'
host { [subdomain, domain_name].compact.join('.') }
end
initialize_with { new("#{username}@#{host}") }
end
end
RSpec.describe User, type: :model do
# ...
it 'should not allow blank host' do
is_expected.to_not allow_value(FactoryGirl.create(:email, host: '')).for(:email)
end
it 'should not allow blank username' do
is_expected.to_not allow_value(FactoryGirl.create(:email, username: '').for(:email)
end
end
On top of that does it really make sense to have a factory to create a email string? why not simply using a string (This is way simpler):
is_expected.to_not allow_value(FactoryGirl.create(:email, host: '')).for(:email)
vs
is_expected.to_not allow_value('test@').for(:email)
If you want to have a consistent :email through your test, why not putting it in the User.
FactoryGirl.define do
factory :user
transient do
username 'user'
subdomain 'mail'
domain_name 'example.com'
host { [subdomain, domain_name].compact.join('.') }
end
before(:create) do |user, evaluator|
user.email = "#{username}@#{host}"
end
end
end
But if the logic belongs to those particular tests, why abstracting all of this in factory instead of simply using a before/callback. As a rule of thumb, I only put logic that will be used across spec files in the factory, everything else is in the proper before/after callback
RSpec.describe User, type: :model do
before :each do
# logic before each test of the file
end
context 'email validation'
before :each do
# logic before each test concerning the email validation
# useful to factor stuff that will be used multiple time
# e.g.
# @email_ok = '[email protected]'
end
it 'should not allow blank host' do
# test specific setup
# useful for stuff only used in this test
# e.g
# email_no_host = 'user@'
# expectation
end
end
end
In short:
What you're doing would work of course. This is not bad practice per se. It just doesn't make much sense IMHO.
EDITED
You could also add an helper inside the test scope in order to not make the model too fat:
RSpec.describe User, type: :model do
context 'email validation'
def generate_email(**opts)
options = {username: 'user', subdomain: 'mail', domain_name 'example.com'}.merge(opts)
username = options[:username]
host = options[:host] || "#{options[:subdomain]}.#{options[:domain_name]}"
"#{username}@#{host}"
end
it 'should not allow blank host' do
is_expected.to_not allow_value(generate_email host: '').for(:email)
end
# Here goes 100 other use of generate_email
end
end
Upvotes: 1