Skip to content
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

removed factories_spec.rb and anywhere it was being used. #259

Closed
wants to merge 2 commits into from
Closed

removed factories_spec.rb and anywhere it was being used. #259

wants to merge 2 commits into from

Conversation

mrageh
Copy link

@mrageh mrageh commented Jan 14, 2014

This is a possible fix for Issue #155

Adam89 added 2 commits January 14, 2014 10:21
@calebhearth
Copy link
Contributor

Cross-commented from #260:

The factory specs running first (thought they add a lot of overhead, which I'm trying to minimize here) is really nice. Despite them never having saved @croaky's butt, they have mine - at least insofar as them running and fast-failing have saved me from trying to figure out why half a test suite is failing. It's nice to have the single failure up front.

@mrageh
Copy link
Author

mrageh commented Feb 3, 2014

Ok thanks for the update caleb, should I close this pull request?

@calebhearth
Copy link
Contributor

@Adam89 this is my opinion, others may have their own. (@jferris, @gylaz)

@jferris
Copy link
Contributor

jferris commented Feb 3, 2014

I haven't gotten any use out of factories_spec.rb yet, and I'd be in favor of removing. Here are my reasons:

  • I don't like waiting for the test environment to boot up an extra time to run rake on one spec file.
  • If you like feedback quickly, there are alternates like failing fast or alternate test runners.
  • The failures from factories_spec.rb are unlikely to be better than the failures from a spec that tries to use an invalid factory. Either way, you'll get the model being saved and the validation failures.
  • We don't have a nice way to reuse these, so we end up copying the file into suspenders apps. The fewer files we have to copy, the better.

@gabebw
Copy link
Contributor

gabebw commented Feb 3, 2014

I agree with Joe's eloquent explanation.

@calebhearth
Copy link
Contributor

The failures from factories_spec.rb are unlikely to be better than the failures from a spec that tries to use an invalid factory. Either way, you'll get the model being saved and the validation failures.

Agreed, except in the case of a feature spec, where the message is just confusing.

@croaky
Copy link
Contributor

croaky commented Feb 3, 2014

I don't like waiting for the test environment to boot up an extra time to run rake on one spec file.

Does Spring (which is in Suspenders) solve this?

If you like feedback quickly, there are alternates like failing fast or alternate test runners.

Are these alternatives or complements?

We don't have a nice way to reuse these, so we end up copying the file into suspenders apps.

How about including them in Factory Girl?

@croaky
Copy link
Contributor

croaky commented Feb 3, 2014

Despite them never having saved @croaky's butt, they have mine

I feel @calebthompson's opinion on this is more valid than mine as he's been working on Rails apps at least 32 hours/week and I'm more like in the single digits.

I'm inclined to merge #260 and see if we can make further changes like including this ability in Factory Girl to reduce files in Suspenders.

@jferris
Copy link
Contributor

jferris commented Feb 3, 2014

Yeah, that could be an interesting factory_girl feature. Something like:

FactoryGirl.lint

If we run this before specs run (ie included in spec_helper.rb or spec/support), it doesn't even need to be constructed as a test.

@calebhearth
Copy link
Contributor

FactoryGirl.lint

I like this.

I don't like waiting for the test environment to boot up an extra time to run rake on one spec file.

Does Spring (which is in Suspenders) solve this?

I know that by virtue of being in the same rake task, the Rails environment doesn't need to be reloaded, which is Spring's benefit. I'm not sure of the overhead of the test environment itself being reloaded, but doubt that Spring effects them.

fast-failing test runners

Right, but we randomize the order so there's no guarantee that the fast-failure will be informative.

I'm all for looking into adding better test support to factory_girl. @joshuaclayton?

@croaky
Copy link
Contributor

croaky commented Feb 8, 2014

FactoryGirl.lint is being done in thoughtbot/factory_bot#609

@mrageh
Copy link
Author

mrageh commented Feb 9, 2014

Can I close this pull request now as thoughtbot/factory_bot#609 solves this issue

@croaky
Copy link
Contributor

croaky commented Feb 10, 2014

@Adam89 I think we still want to figure out a way to run FactoryGirl.lint inside the Rails app. Should this be in the rake task similar to the existing version (but deleting the factories_spec.rb file)?

@joshuaclayton
Copy link

To update this ticket, FactoryGirl.lint is available in factory_girl 4.4.0, released this morning!

@mrageh
Copy link
Author

mrageh commented Feb 11, 2014

Cool that means I can close this pull request right?

@calebhearth
Copy link
Contributor

Yeah, thanks @Adam89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants