[11.x] Constrain key when asserting database has a model

[patrickomeara]

There are currently three options to pass as the first argument to assertDatabaseHas() and assertDatabaseMissing()

• the table name (string)
• the class (string)
• a model (instance)

$pat = User::factory()->create(['name' => 'Pat']);

$this->assertDatabaseHas('users', ['name' => 'Pat']); // pass
$this->assertDatabaseHas(User::class, ['name' => 'Pat']); // pass

$cinderella = User::factory()->create(['name' => 'Cinderella']);

$this->assertDatabaseHas($cinderella, ['name' => 'Pat']); // pass

This PR adds a constraint on the model's key when an eloquent model is used so the last assertion above would fail.

Breaking Change

Like the above example, if a test suite currently asserts a database row contains data against a different model instance that test will now fail. However, I would argue that the test gives a false positive and should change to use the class string or the correct model instance.

$this->assertDatabaseHas(User::class, ['name' => 'Pat']); // pass

$this->assertDatabaseHas($pat, ['name' => 'Pat']); // pass

Additionally, passing an empty model instance is an anti-pattern as until it is saved, the database does not have that model.

Currently the following test would pass:

$pat = User::query()->find(1);
$cinderella = new User(['name' => 'Cinderella']);

$this->assertDatabaseHas($cinderella, ['name' => 'Pat']); // pass
$this->assertDatabaseMissing($pat, ['name' => 'Cinderella']); // pass

// Using User::class for these has a clearer intention.
$this->assertDatabaseHas(new User, ['name' => 'Pat']); // pass
$this->assertDatabaseMissing(new User, ['name' => 'Cinderella']); // pass

This change will only break those specific tests not the application logic.

[devfrey]

I like the idea, but this is breaking and shouldn't be targetted at 11.x. Changing the signature of HasInDatabase::__construct() alone is breaking.

I also don't think it makes sense to make $data optional in assertDatabaseHas() and assertDatabaseMissing(). We already have assertModelExists() and assertModelMissing() respectively, that assert without the data.

[patrickomeara]

The breaking changes are not in the application layer and would only affect those that extend HasInDatabase or those that could afford to have some clearer intention around their tests. ie using the class instead of a new instance, or using the correct instance.

This change will also uncover any false positives that may exist in a test suite due to those reasons.

This could easily target the next major release, however it needs to be weighed up on the additional work load around that release (upgrade docs, conflicts, five months of rebasing) versus how many tests this will affect. Remembering that it's a test only change, no application will break from this change.

It's not uncommon for a minor release to break tests, here are some examples I can think of:

• Allow dusk attribute selectors to be chained dusk#1034 This broke my dusk selectors, but it's the correct change, they shouldn't have spaces
• [11.x] Don't touch BelongsTo relationship when it doesn't exist #52407 If your tests counted queries, this would've broken them.

Anyone using a new model instance can replace it with the class string using the following regex.

s/->assertDatabase(Has|Missing)\((\s*)new ([\w\\]+)(?:\(\))?,/->assertDatabase$1\($2$3::class,/

Regarding, assertModelExists() and assertModelMissing(); they are both database checks but that isn't overly clear. Is it just checking the exists attribute of the model?

$user = new User(['exists' => true]);

$this->assertModelExists($user); // am I using this correctly?

$this->assertDatabaseHas($user); // fail, much clearer

A common pattern I see in feature tests is making a model using a factory, sending the request then refreshing the instance and asserting it's attributes, this cleans that up.

// arrange
$user = User::factory()->create(['name' => 'Pat', 'role' => 'developer']);

// act
$this->patchJson(route('users.update', $user), ['name' => 'Cinderella'])
    ->assertOk();

// refresh and assert
$user->refresh();
$this->assertSame('Cinderella', $user->name);
$this->assertSame('developer', $user->role);

// or just assert the database has it
$this->assertDatabaseHas($user, ['name' => 'Cinderella', 'role' => 'developer']);

[samlev]

People can also use $model::class if they want a minimal change to their tests.

[samlev]

Instead of changing the signature here, would it be better to do something like this in the assert methods?

if ($connection instanceof Model && $connection->getKey()) {
    $data = array_merge($data, [$connection->getKeyName() => $connection->getKey()]);
}

So that it's just injecting the model's key into the data that it's checking for if it isn't already set.

[patrickomeara]

That's definitely possible, but would result in duplicate code in both assertDatabaseHas() and assertDatabaseMissing() methods, as all roads lead to HasInDatabase I moved it there.

[taylorotwell]

I also don't really love injecting $model here. Doesn't feel like this class should have anything to do with Eloquent.

[patrickomeara]

Ok, let me refactor this a little.

[patrickomeara]

I have left HasInDatabase as is. This should be a little more palatable. Thanks for the push in the right direction @samlev

[taylorotwell]

Thanks 👍