Lint Factories

[cbrunsdon]

We should run all our factories through FactoryGirl.lint, as discussed on Slack I highly suspect we won't pass.

This should be a task accomplish-able by anyone with medium ruby/rails/factory girl experience, but could get very tricky depending on which factories don't build.

[wgriffiths]

I will start looking into this

[travisp]

Yes, I just tried importing spree factories into an app to use for testing, and the app lints all factories by default when all specs are run. It came up with these errors (after I modified the default user factory to have an 8 character password):

       * adjustment - Validation failed: Order can't be blank (ActiveRecord::RecordInvalid)
       * tax_adjustment - Validation failed: Order can't be blank (ActiveRecord::RecordInvalid)
       * customer_return_without_return_items - Validation failed: Return items can't be blank (ActiveRecord::RecordInvalid)
       * image - No such file or directory @ rb_sysopen - /Users/travis/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/solidus_core-1.1.0/spec/fixtures/thinking-cat.jpg (Errno::ENOENT)
       * refund - Validation failed: Amount is greater than the allowed amount. (ActiveRecord::RecordInvalid)
       * base_shipping_method - Validation failed: Calculator can't be blank (ActiveRecord::RecordInvalid)
       * stock_packer - undefined method `save!' for #<Spree::Stock::Packer:0x007feff84d6378> (NoMethodError)
       * stock_package - undefined method `save!' for #<Spree::Stock::Package:0x007fefdd080320> (NoMethodError)
       * stock_package_fulfilled - undefined method `save!' for #<Spree::Stock::Package:0x007feff84ae490> (NoMethodError)
       * stock_item - Validation failed: Variant has already been taken (ActiveRecord::RecordInvalid)
       * stock_movement - Validation failed: Variant has already been taken (ActiveRecord::RecordInvalid)
       * stock_transfer_with_items - Validation failed: Code has already been taken (ActiveRecord::RecordInvalid)
       * receivable_stock_transfer_with_items - Validation failed: Code has already been taken (ActiveRecord::RecordInvalid)
       * store_credit_event - PG::NotNullViolation: ERROR:  null value in column "action" violates not-null constraint
       DETAIL:  Failing row contains (4, 2, null, 100.00, 0.00, 2-SC-20140602164814476128, null, null, null, 2015-12-17 03:38:31.335739, 2015-12-17 03:38:31.335739, null).
       : INSERT INTO "spree_store_credit_events" ("store_credit_id", "amount", "authorization_code", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id" (ActiveRecord::StatementInvalid)
       * global_zone - Validation failed: Name has already been taken (ActiveRecord::RecordInvalid)

One or two of them look like expected errors, and a couple are only conflicts when multiple factories are run (although it would be nice if uniqueness wasn't violated in that case). A number of the others look like actually invalid factories.

[mamhoff]

Right now, there is only five invalid factories left:

* customer_return_without_return_items - Validation failed: Return items can't be blank (ActiveRecord::RecordInvalid)
* stock_packer - undefined method `save!' for #<Spree::Stock::Packer:0x007f7f7bed2248> (NoMethodError)
* stock_transfer_with_items - Validation failed: Code has already been taken (ActiveRecord::RecordInvalid)
* receivable_stock_transfer_with_items - Validation failed: Code has already been taken (ActiveRecord::RecordInvalid)
* store_credit_event - SQLite3::ConstraintException: NOT NULL constraint failed

Some of these are probably not easy fixes. Who takes on the challenge?

[AlessioRocco]

Working on it!

[bbuchalter]

@AlessioRocco what progress have you made here? I also started looking at this.

As I investigated further, I came to think more and more that FactoryGirl may not be the right tool for us. Take for example, the simple act of adding a line item to order. Using FactoryGirl to build or create a line item for that order makes no sense as that's the job of OrderContents#add.

Now if we don't care about that extra behavior, that's fine. But then I'm not sure we shouldn't just be using test doubles and avoid the complexities and performance hits of FactoryGirl.

Right now we seem to be somewhere in between those two worlds: we don't really execute the same behavior in tests as we would in a store, but we do try and recreate some of that behavior.

What are your thoughts?

[AlessioRocco]

@bbuchalter for now I'm working on a good way to break the specs or notify the developer if we have some "bad" or not tested factories, for now I've just added the linter 670c105 at the beginning of our test suite but more I understand about Factory in Solidus and more I think it's not a good solution, this is a duplication of what we do with a working factory shared example, basically the linter just try to create a factory and raise an error if it's fail, we do the same with this code https://github.com/solidusio/solidus/blob/master/core/spec/support/concerns/working_factories.rb#L7. A better way maybe could be to understand what factories are tested and then in some way notify the developer if there are some factories that aren't tested, currently I'm working on it.

About your thoughts on "FactoryGirl may not be the right tool" I understand your point of view and I agree, for now I think that for mimic the store behavior we could use the method that Solidus give to us in the Factory as much as possible, for example the line_items created by :order_with_totals factory https://github.com/TommyJohnWear/solidus/blob/e3b043815e694f669b45947930049e99ab38fe40/core/lib/spree/testing_support/factories/order_factory.rb#L22-L30 could be changed by OrderContents#add method by create a Variant and then #add it to the order, I understand that the problem is that #add doesn't accept the price for the line_item, maybe we can add it as an extra option for the #add method. This way to proceed could also help us thinking about our API and improve it.

---

@mamhoff I think that some factories are invalid just because the linter was run two or more times, for example the : stock_transfer factory work if you run the linter just one time but fail if you run it two time, in my PR I've changed it a little 138d656 by adding a sequence, I did it just for this and I'm not sure that it's needed.

[bbuchalter]

@AlessioRocco thanks for the thoughtful reply. I'm glad to hear that you feel the idea of using Solidus's API for setting up state for factories is a good one. However, on the other hand, I don't want to be optimizing factories in isolation. I'd rather see them improved in the context of making other specs more correct, or more maintainable. Does that make sense? Do you have any specs in mind that would benefit greatly from improved factories?

[mamhoff]

@bbuchalter I'm not sure we should be using the Solidus API for setting up test data, mostly because those factories are used in client stores where they might very well use external services to set up shipments or taxes, which would slow down many client test suites. This would mean sample data would actually test production code, leading to hard-to-debug spec setups I think.

[seand7565]

Hey gang, this issue is almost 5 years old, and hasn't had an update in almost 3 years. I think we should consider closing this - if it's still something we want to work on, maybe we can create a new issue outlining the current state of things. I imagine almost everything written here is completely outdated. Thanks!

[seand7565]

I believe #3647 should close this issue. All factories (except one, notated in the PR) pass linting 🎉