nothingworksinc / ticketbeast

Back to the lesson videos:
https://course.testdrivenlaravel.com/lessons
552 stars 11 forks source link

Potential issue with model factories (Refactoring for speed) #21

Open sileence opened 7 years ago

sileence commented 7 years ago

I really enjoyed this lesson and I've been doing something similar, however I've spotted a potential issue when working with model factories: some of them might create new registers in the database, i.e.:

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

What happens there? Should I assign a fake ID to avoid the creation of the user object?

sileence commented 7 years ago

That's exactly what the closure does @liefie, you don't need the ternary operator. But back to square one: we don't have an ID if we don't store the code in the database.

liefie commented 7 years ago

Learn something new every day!

I understand your question now, you're creating the user, but not creating the Message as you're going to call factory()->make();

Perhaps adding a state?

allowing $user->messages()->save(factory(App\Message::class)->make();, factory(App\Message::class)->make() and factory(App\Message::class)->states('withUser')->create();

$factory->define(App\Message::class, function (Faker\Generator $faker) {
    return [
        'message' => $faker->sentence,
    ];
});

factory->state(\App\Message::class, 'withUser', function(\Faker\Generator $faker) {
    return [
        'user_id' => function () {
            return factory(\App\User::class)->create()->id;
        },
    ];
});
sileence commented 7 years ago

That could be, although I don't like the extra effort. I'm thinking in sending a Pull request to Laravel that could solve this in another way, if another solution doesn't exist yet.

Maybe you could do:

$factory->define(App\Message::class, function (Faker\Generator $faker) {
    return [
        'message' => $faker->sentence,
        'user' => function () {
            return factory(\App\User::class)->create();
        },
    ];
});

Maybe if make() is called then all the relationships will be "made" as well instead of "created"

For now I'm just not using factories but manually calling new Message

adamwathan commented 7 years ago

If you are using create to create a Message, you'll likely want the User created as well, since a Message isn't really "valid" without an associated User, especially if you are enforcing foreign key constraints.

However, if you're using make to build an in-memory Message that doesn't need to be saved to the database for the purposes of the test, you can pass in an arbitrary user_id or create a state like @liefie suggested:

$message = factory(Message::class)->make(['user_id' => 0]);

...or using a state, you could do something like:

$factory->state(Message::class, 'stubbed', function () {
    return ['user_id' => 0];
});

$message = factory(Message::class)->states('stubbed')->make();
sileence commented 7 years ago

Thank you @adamwathan and @liefie. As I mentioned above I'm thinking in a built-in solution for Laravel, does that make sense?

adamwathan commented 7 years ago

@sileence I wrote a factory package many years ago that solved the problem the way you are proposing but haven't really maintained it since Laravel got it's own factories:

https://github.com/adamwathan/faktory#relationships-and-build-strategies

It's tricky to get it to always behave exactly as if the relationships were saved, so I haven't bothered with the approach much and instead just create what I need in the test database.

I wrote about a similar approach you can sometimes use here though:

https://adamwathan.me/2016/08/04/stubbing-eloquent-relations-for-faster-tests/

sileence commented 7 years ago

I'll see if I can come up with something, otherwise I'll follow your previous advice about the states!