Requirable factories #2 #627
Conversation
|
Isn't setting Otherwise I love the factory specs. |
|
I'm also not really excited about the env check, I know it means we have a test to make sure that factories would be individually loadable, but I'm not sure thats worth the potential extra issues by controlling loading with ENV settings. Will think about it. Tests on our factories if fantastic though, thanks for the effort. The huge reliance on our factories by ourselves and other projects without any reasonable suite has always been a black mark on the project IMO. |
|
The I'll remove it - for knowing whether any factories still break, we could do one of two things:
If anyone has a better idea on how to test |
|
We could look to do it outside of our specs as well just as a CI task. While individually requirable is nice, I'd definitely say not worth confusing up our specs. I'd even say we try waiting on backlash for a while. We'll also be watching PR's to make sure we're not adding dependencies to factories without including them, so we might be optimising for a problem we won't have. |
|
Switch removed :) |
|
As this is just going into Kevin's original PR I am 👍 |
|
Whoops, sorry, missed where it was being targeted it to. |
| @@ -0,0 +1,9 @@ | |||
| require 'factory_girl' | |||
There was a problem hiding this comment.
I think this file would be better under testing_support/factories (but I don't feel that strongly about it)
There was a problem hiding this comment.
I wouldn't mind doing that, but that was Kevin's decision, and I didn't want to touch his stuff. @Sinetheta should the file be moved?
|
👍 Regardless of my suggestion. This is great. |
|
👍 thanks! Would you mind rebasing and resolving the merge conflicts? Looks like we're ready to merge after that. |
Our goal is to break up all of the factories so that they can be required independently of each other so we need these sequences available outside of factories.rb which requires all of the factories.
Doing this class eval in factories.rb would have create an unreasonable dependency on this file for the ShippingMethod factory
Requiring files based on environment variables makes the test suite vulnerable to sort order. Yanking.
|
@jordan-brough rebased! |
|
Perfect, thanks again! |
In #627 we removed the hack around `Spree::Zone.global`. This change is currently missing in the change log.
This PR builds on #623 (and includes @Sinetheta s work). I went through all factories, set up tests that don't require
testing_support/factories, and made sure all factoriesbuildandcreate. As the factories are IMO code, and a pretty important part of the framework, I suggest to leave the tests in the repo. They can later be used to improve the factories even more. I do understand they seem pretty bogus at times, but having a systematic way of testing factories is a nice thing.