lukeraymonddowning / poser

Create class based model factories in Laravel applications in seconds.
MIT License
277 stars 10 forks source link

Automated `create` functionality added #44

Closed lukeraymonddowning closed 4 years ago

lukeraymonddowning commented 4 years ago

Been thinking about this for a little while. I often forget to call create() or () on the factories when writing tests (and I built it!), so I imagine there are others who forget too. This is especially the case in for[RelationshipMethodName] examples, where I forget to create the owning model.

As such, I've been working on this feature, which will automatically call create on factories when a property or function of the built instance is called which doesn't exist in the factory. If it still doesn't exist in the created model/collection, we throw the ModelNotBuiltException as we have before.

Here is an example:

/** @test */
    public function it_works_with_for_relationships()
    {
        $customers = CustomerFactory::times(3)->forUser(UserFactory::new());

        $customers->each(
            function ($customer) {
                $this->assertInstanceOf(User::class, $customer->user);
            }
        );
    }

You can see we didn't have to call create() on either the CustomerFactory or the UserFactory, but the test passes fine.

@AlanHolmes, @andreich1980 do you guys see a value in this or potential pitfalls?

The only case it wouldn't work for is something like $this->assertCount(n, Customer::all()) but I don't necessarily think that is a very good way of testing anyway.

andreich1980 commented 4 years ago

The feature looks nice. 👍 What about Customer::count()? Wouldn't this work as well?

andreich1980 commented 4 years ago

By the way, you have some unused classes in the Factory.php: use App\User;, use App\UserFactory;, use App\CustomerFactory;

lukeraymonddowning commented 4 years ago

The feature looks nice. 👍 What about Customer::count()? Wouldn't this work as well?

Very true. Basically falls short for any test where you access the models indirectly. My question is: is that reason enough to pass on this? Would it cause too much confusion?

AlanHolmes commented 4 years ago

Seems good to me, esp for the relationships where you have to remember if ->create is needed or not depending if it is with or for

andreich1980 commented 4 years ago

Anyway you could leave this as a feature. And those who wants to use ::all() badly - they would just need to call ->create() or (). That's it.

lukeraymonddowning commented 4 years ago

Okay, the consensus seems to be positive. We'll go with it. I'm going to improve the documentation around it, then merge in. Thanks guys 👌