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

[Proposal] Better management of subfactories than closure #485

Closed
ThibaudDauce opened this Issue Mar 24, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@ThibaudDauce
Copy link

ThibaudDauce commented Mar 24, 2017

TLDR;

We are hardcoding ->create()'s inside of factories that we can call "->make()" on. Therefore, calling "->make()" isn't a sure thing nothing will be persisted to the db...


One very common way to use subfactories in Laravel is to make a Closure and call the factory inside:

$factory->define(App\Phone::class, function (Faker\Generator $faker) {
    return [
        'user_id' => function() {
            return factory(App\User::class)->create()->id;
        },
        'phone' => $faker->phoneNumber,
    ];
});

The main disadvantage of this approach is that the create() method is hard-coded. If I want to use this factory without the DatabaseMigrations and a call to make() my testing database will be polluted.

factory(App\Phone::class)->make();
$this->assertEquals(0, App\User::count()); // fail, there is one user in my database

I think the best API for the users is to use the same key as the name of the relation:

$factory->define(App\Phone::class, function (Faker\Generator $faker) {
    return [
        'user' => factory(App\User::class),
        'phone' => $faker->phoneNumber,
    ];
});
$phone = factory(App\Phone::class)->make();
$this->assertEquals(0, App\User::count()); // success, the make function is called on the factory
$this->assertInstanceOf(App\User::class, $phone->user); // success, the relation is populated with `setRelation`
$this->assertNull($phone->user_id); // success, this is the only drawback

It will not work well with the raw() method. The array will not contain neither a user key, nor a user_id key.

If it's a wanted feature, I can submit a pull request. The FactoryBuilder will inspect the model to get the Relation, create or make a new model and set the appropriate keys (with getOwnerKey(), getForeignKey()…).

PS: I noticed there is no test for the Factory and FactoryBuilder… :-(

PS2: it could also work with Collection for HasMany relationships:

$factory->define(App\User::class, function (Faker\Generator $faker) {
    return [
        'phones' => factory(App\Phone::class, 3),
    ];
});

PS3: it could also work with suboverride attributes:

$phone = factory(App\Phone::class)->make([
    'user' => [
        'email' => 'john@example.com',
    ]
]);
$this->assertEquals('john@example.com', $phone->user->email); // success

@ThibaudDauce ThibaudDauce changed the title Better management of subfactories than closure [Proposal] Better management of subfactories than closure Mar 24, 2017

@Dylan-DPC

This comment has been minimized.

Copy link

Dylan-DPC commented Mar 24, 2017

You are adding more complexity and achieving marginal gains through this approach

@Garbee

This comment has been minimized.

Copy link

Garbee commented Mar 24, 2017

Why not structure things where you simply call make on things with relations and in your logic do the relating just like you would in your application. Yes, a little extra manual code but it doesn't force any logic into the factories that shouldn't really happen there.

Factories are about creating a given model as much as it can, not completely. If you keep doing this in factories then in your tests you can end up doing a lot more work than necessary to test given cases.

@Dylan-DPC

This comment has been minimized.

Copy link

Dylan-DPC commented Mar 25, 2017

My only concern is you are using "magic".

Why not:

return [
        'user_id' => factory(App\User::class),
        'phone' => $faker->phoneNumber,
    ];
@ThibaudDauce

This comment has been minimized.

Copy link
Author

ThibaudDauce commented Mar 27, 2017

@Garbee with this new feature my tests will be much cleaner because I will not need to build my models manually and just use factories.

@Dylan-DPC I think this solution use the same amount of magic than the relations in Laravel. It's the same approach of doing $user->phone to access the related model.

@calebporzio

This comment has been minimized.

Copy link

calebporzio commented Mar 28, 2017

I dig this - seems like a nice idea.

I'm not sure how I feel about referencing the relation name "user" instead of hard fields in the array like "user_id" -> but I understand thats kind of the point of this.

At the end of the day, I think we agree on the problem at hand:

We are hardcoding ->create()'s inside of factories that we can call "->make()" on. Therefore, calling "->make()" isn't a sure thing nothing will be persisted to the db...

@Dylan-DPC

This comment has been minimized.

Copy link

Dylan-DPC commented Mar 28, 2017

@ThibaudDauce yes but you are using magic where it may not be required. Just adds more complexity

@skyrpex

This comment has been minimized.

Copy link

skyrpex commented Mar 28, 2017

Just to add more background: you can use a factory state to include relations. For example:

$factory->define(App\Project::class, function (Faker\Generator $faker) {
    return [
        'name' => $faker->name,
    ];
});
$factory->define(App\Project::class, 'with_user', function (Faker\Generator $faker) {
    return [
        'user_id' => function () { /* ... */ },
    ];
});

You can include different states for each relation if you really need to keep them separate.

@ThibaudDauce

This comment has been minimized.

Copy link
Author

ThibaudDauce commented Mar 28, 2017

@calebporzio You totally understood my point. I'm not entirely sure of using the relation name is the correct solution but the problem of hard-coding create() inside a factory with a make() method is here. Maybe something else could be possible… I updated my initial post with your sentence in a TLDR :-)

@skyrpex yes, I know it's possible but it doesn't resolve the problem of create() inside make()

@skyrpex

This comment has been minimized.

Copy link

skyrpex commented Mar 28, 2017

yes, I know it's possible but it doesn't resolve the problem of create() inside make()…

@ThibaudDauce Indeed, but... does it make sense to have relations without actual IDs?

I always use DatabaseMigrations whenever model factories are involved, no matter if I use make or create.

@calebporzio

This comment has been minimized.

Copy link

calebporzio commented Mar 28, 2017

@skyrpex - nice!! I like that and will certainly start using that personally.

@ThibaudDauce - I agree it doesn't solve the problem, but it certainly helps to not have phantom "create()"s in a model factory. I think I probably don't have a deep enough understanding of relationships and if / how they exist without persistence - so I'll let those people weigh in.

@Dylan-DPC

This comment has been minimized.

Copy link

Dylan-DPC commented Mar 28, 2017

@calebporzio

but it certainly helps to not have phantom "create()"s in a model factory.

any reason for that?

@calebporzio

This comment has been minimized.

Copy link

calebporzio commented Mar 28, 2017

@Dylan-DPC - ya, it destroys the promise of being able to ->make() and not have anything persist to the DB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.