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

Issue with create_default and traits #80

Closed
dgilperez opened this Issue Jun 20, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@dgilperez

dgilperez commented Jun 20, 2018

I have this factory, with a trait that allows changing the association general definition like this:

FactoryBot.define do
  factory :idea do
    association :status, :default

    trait :bla do
      association :status, :bla
    end
  end
end 

When I use FactoryDefault to define a default status like this in a shared context:

shared_context 'My context' do  
  let_it_be(:status) { create :status }

  before do
    set_factory_default :status, status
  end
end

And write my spec:

include_context 'My context'

it do
  create :idea #=> is created with the default :status from shared context
  create :idea, :bla #=> is ALSO created with the default :status from shared context
end

So in practice trait :bla is not used at all, set_factory_default is overriding that. This was, at least, surprising and unexpected. I expected the trait behaviour to preveal, since it is explicitly declaring the association.

Is this intented behaviour or is this a 🐛?

Ruby Version: 2.3.7
Framework Version: Rspec 3.6
TestProf Version: 0.5.0

@palkan

This comment has been minimized.

Owner

palkan commented Jun 20, 2018

Is this intented behaviour or is this a 🐛?

It's an intended behaviour. FactoryDefault doesn't take into account traits.

I think, we can add this functionality in the following way:

# make this record default for the specified traits
create_default(:idea, :my_trait, **params, preserve_traits: true)
# or
set_factory_default(:idea, :my_trait, obj)

# we can set `preserve_traits` globally
TestProf::FactoryDefault.preserve_traits = true

# by default should be false for backward compatibility

@palkan palkan added the enhancement label Jun 20, 2018

@palkan

This comment has been minimized.

Owner

palkan commented Jun 22, 2018

@dgilperez Would you like to propose a PR, btw?

@palkan palkan added the help wanted label Jun 22, 2018

@dgilperez

This comment has been minimized.

dgilperez commented Jun 22, 2018

So I was thinking about the proposed interface, and I was thinking more in the other direction:

My thinking is that having as we have several different traits per factory, playing with associations, your proposal of defining different defaults for different traits will end up in the need to define one default per trait, and the need to keep remembering that (I will keep forgetting for sure 😄). I'd prefer a way to get my expected behaviour above.

Something like use default factory, unless you define it explicitly in a trait.

# default false for backward compatibility
TestProf::FactoryDefault.preserve_traits = true 

set_factory_default(:idea, obj)

create :idea_thing # =>  sets obj as default
create :idea_thing, :trait_that_changes_idea_association # =>  does not set obj as default

(Maybe I didn't understand your proposal).

I'd definitely like to contribute. But I will not be able until, at least, August. I'm just getting offline for some days, so no rush on my side anyway. If someone wants to get ahead, that'd be great as well.

@palkan

This comment has been minimized.

Owner

palkan commented Jun 22, 2018

defining different defaults for different traits will end up in the need to define one default per trait, and the need to keep remembering that
I'd prefer a way to get my expected behaviour above.

Agree. Just adding preserve_traits should be enough. No trait => use default, has traits => fallback to the default factory behaviour.

Mordorreal added a commit to Mordorreal/test-prof that referenced this issue Jun 24, 2018

Add possibility to preserve traits
Now the user can set FactoryDefault.preserve_traits to true and the
factory default will fallback to the default factory behaviour.

Ref: palkan#80

Mordorreal added a commit to Mordorreal/test-prof that referenced this issue Jun 24, 2018

Mordorreal added a commit to Mordorreal/test-prof that referenced this issue Jun 24, 2018

Add possibility to preserve traits
Now the user can set FactoryDefault.preserve_traits to true and the
factory default will fallback to the default factory behaviour.

Ref: palkan#80

Mordorreal added a commit to Mordorreal/test-prof that referenced this issue Jun 24, 2018

Vasfed added a commit to Vasfed/test-prof that referenced this issue Jun 26, 2018

Vasfed added a commit to Vasfed/test-prof that referenced this issue Jun 26, 2018

@palkan palkan closed this in #83 Jun 28, 2018

palkan added a commit that referenced this issue Jun 28, 2018

[Fix #80] Add ability to preserve traits (#83)
* [Fix #80] Ability to revert to default behavior when have traits on association

* Use kwargs in `set_factory_default` and store options along objects

* Enable non-default Style/ReturnNil cop

* Remove unused methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment