Railtie overrides user-specified factories directory #64

Closed
wants to merge 4 commits into from

2 participants

@urbanautomaton

I recently noticed that our generated factories were being placed in
spec/factories, despite our having the following line in our
config/application.rb:

g.fixture_replacement :factory_girl, :dir => "test/factories"

I tracked this down to the railtie included with factory_girl_rails,
which overrides any user-specified configuration. It was introduced in
this commit: 97222f2

I've added some features and updated the behaviour, so the auto-added
configuration is only applied if none is already specified. It also no
longer specifies the factories directory to be spec/factories, but
leaves this up to the generator default. I think this is less confusing
(it certainly took me a while to work out why both my configuration and
the generator default was being ignored).

Just let me know if there's anything else required for this patch to be
accepted... :)

Cheers,
Simon

urbanautomaton added some commits Jun 18, 2012
@urbanautomaton urbanautomaton Add failing feature for fixture directory override
For projects configured with rspec as the test framework, the railtie
overrides any user-specified fixture directory that might be included in
config/application.rb.

This patch modifies the required behaviour so that if such configuration
is detected, it is not overridden. It also expects the auto-added
configuration to match the generator's default of 'test/factories'.
f2cf240
@urbanautomaton urbanautomaton Fix overridden fixture config in railtie
The railtie now only auto-adds fixture_replacement configuration if none
is detected. It also does not specify a fixtures directory, deferring
this to the generator's default instead.
0d94362
@urbanautomaton urbanautomaton Better check for option presence 4583093
@urbanautomaton

Actually, I've just noticed that the original behaviour of this railtie (which I've preserved) was that if the configured test framework is not Rspec, it forces it to be Test::Unit. Presumably this would override the configuration of unsuspecting minitest and shoulda users.

What's preferable here? Anticipate every test framework that people might use, and attempt to configure factory_girl appropriately? Or just leave the Rspec auto-config in, and leave everyone else to their own devices?

@urbanautomaton

Apologies for bumping this, but I've added a commit to stop the railtie auto-configuring the test framework to Test::Unit unless rspec is already configured. Is there anything else I can do to make this suitable for merging?

@urbanautomaton

Hi @joshuaclayton - sorry to directly copy you in but seeing as there's been a few releases since I opened this pull request, I was wondering if there's anything else I can do to make this ready to merge. I know it's a minor issue but overwriting configuration in this way is pretty confusing to the average user (well, it confused me, anyway :))...

@joshuaclayton
thoughtbot, inc. member

hey @urbanautomaton, sorry I didn't get back to you sooner - I'll look at this again in the next week or so. I apologize for letting this drop off my radar! We've been gunning for 4.0 and with it getting released last week, I'll be able to catch up on this and some other things in the factory_girl_rails gem.

Again, thanks for your patience! I'll be sure to let you know if I need anything.

@joshuaclayton
thoughtbot, inc. member

This is fixed in 97222f2

@urbanautomaton

Thanks very much. :)

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