Include DatabaseCleaner usage when demoing FactoryGirl.lint #611

Merged
merged 1 commit into from Feb 26, 2014

Conversation

Projects
None yet
7 participants
Owner

joshuaclayton commented Feb 13, 2014

Because built factories create associated records, the database may not be
empty when the suite is run. This encourages developers to start
DatabaseCleaner and clean after linting is complete to ensure a clean
database.

cc @jferris

Owner

jferris commented Feb 13, 2014

Can we avoid this issue by having FactoryGirl.lint use build_stubbed?

Owner

joshuaclayton commented Feb 14, 2014

@jferris that's a good question; with validations and other things happening in the model, I'm not confident build_stubbed would fix every issue, whereas clearing out the database would, I think.

Alternatively, we could provide a rake task in factory_girl_rails which runs lint and encourage developers to add the task as a prereq for tests; that way, it's run independently of the test suite and can be run on its own (would we want to ensure a test env?).

I like the fact that factory_girl has introduced something to try to test factories easily. We have a test that runs in CI (needs work, but here is a gist) to test creates for existing models, but lint found those we had missed- factories that no longer worked that we weren't using because tables and models went away.

But:

  1. I don't think that just building factories is sufficient for factory testing. It is a good tool to have available, but it is just as valid to test creates and have DB cleaner clean that. So, if anything lint should take an optional set of strategies to test, and default to build, e.g. FactoryGirl.lint :build, :create
  2. On the opposite end, even lint building all factories can cause unnecessary red flags. Some factories are used as dependencies to other factories and not really called by themselves in tests, even if the assumption is there that they should be buildable by the fact that they exist- so why test build for something unnecessarily? For this reason, I do not think that it should be the default in factory_girl_rails unless there were an obvious and stated config option to be able to tell it to only do a subset or none at all, but that still seems non-obvious. To remedy this, I think that options passed to lint like :include and :exclude that take arrays of factory names may be good, even if that doesn't solve having to test traits, etc. Maybe even additional an additional option on/in the factories definitions would be a good way to indicate what strategies should not be tested by lint- I dk.

Sorry I did a dupe PR for this, and thanks for factory_girl and all of your work. Much appreciated!

@joshuaclayton joshuaclayton Include DatabaseCleaner usage when demoing FactoryGirl.lint
Because built factories create associated records, the database may not
be empty when the suite is run. This encourages developers to start
DatabaseCleaner and clean after linting is complete to ensure a clean
database.

Closes #619, #611, #620
7f31ba9

@joshuaclayton joshuaclayton merged commit 7f31ba9 into master Feb 26, 2014

joshuaclayton deleted the jc-improve-lint-docs branch Feb 26, 2014

Cool! Thanks, @joshuaclayton.

Can confirm this is still happening.

Based on @joshuaclayton's patch, this is a workaround, that can be placed in the spec_helper.rb:

config.before(:suite) do
begin
DatabaseCleaner.start
FactoryGirl.lint
ensure
DatabaseCleaner.clean
end
end

Hi

I would say it is better to create separate spec file spec/lint_spec.rb with:

require 'rails_helper'

RSpec.describe "Lint" do
  it "FactoryGirl" do
    FactoryGirl.lint
  end
end

In this case the references to DatabaseCleaner not needed, because the default cleaning strategy will be applied (for example config.use_transactional_fixtures = true will work as well).
And you can run it only on purpose, ( but not before any test run which may don't use factories at all)

Owner

joshuaclayton replied Jul 25, 2014

@mshytikov I dig this idea - especially when there are other mechanisms for stubbing external requests and other similarly tricky issues. Thanks for sharing!

Thanks a lot @mshytikov! Perfect solution. I also want to point out that it took me a bit of time to track down FactoryGirl.lint as the source of this problem, and even then it was on a hunch. @joshuaclayton maybe you could consider adding a warning to the lint section in the FactoryGirl Getting Started doc?

Owner

joshuaclayton replied Sep 24, 2014

@AWaselnuk did you have some text in mind that would've been helpful for you? If so, could you put together a PR and we can discuss there? I'm completely onboard with improving the experience!

@joshuaclayton I just saw this on GETTING_STARTED.md master branch:

After calling FactoryGirl.lint, you'll likely want to clear out the database, as built factories will create
associated records. The provided example above uses the database_cleaner gem to clear out
the database; be sure to add the gem to your Gemfile under the appropriate groups.

That's perfect. It mentions the expected side effect of linting and provides a possible solution. Thanks!

e2 replied Jan 25, 2015

@joshuaclayton, @mshytikov - that rspec example would be perfect as a default generated test in factory_girl_rails

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