Level 61
I usually do only database assertions cause I find them cleaner.
Dec 29, 2020
11
Level 54
Writing Test Assertions for Feature vs Unit
Hey all I’m looking for assistance with knowing what assertions should be Feature/Unit tests.I'm following along with the current series Jeffrey Way is doing on Testing and trying to think out what assertions I need but I tend to favor more of assert more than whats needed so I'd like some thoughts.
/**
* @test
* @dataProvider administrators
*/
public function invoke_activates_an_unactivated_user_and_redirects($administrators)
{
$now = now();
Carbon::setTestNow($now);
$user = User::factory()->unactivated()->create();
$this->actAs($administrators)
->patch(route('users.activate', $user))
->assertRedirect(route('users.index'));
tap($user->fresh(), function ($user) use ($now) {
$this->assertEquals(UserStatus::ACTIVE, $user->status);
$this->assertCount(1, $user->activations);
$this->assertEquals($now->toDateTimeString(), $user->activations->first()->started_at->toDateTimeString());
});
}
<?php
namespace App\Http\Controllers\Users;
use App\Http\Controllers\Controller;
use App\Http\Requests\Users\ActivateRequest;
use App\Models\User;
class ActivateController extends Controller
{
public function __invoke(User $user, ActivateRequest $request)
{
$user->activate();
return redirect()->route('users.index');
}
}
<?php
namespace App\Models\Concerns;
use App\Exceptions\CannotBeActivatedException;
use App\Exceptions\CannotBeDeactivatedException;
use App\Models\Activation;
trait CanBeActivated
{
/**
* Activate a model.
*
* @param string|null $startedAt
* @return void
*/
public function activate($startedAt = null)
{
throw_unless($this->canBeActivated(), new CannotBeActivatedException('Entity cannot be employed. This entity is currently employed.'));
$startDate = $startedAt ?? now();
$this->activations()->updateOrCreate(['ended_at' => null], ['started_at' => $startDate]);
$this->updateStatusAndSave();
}
}
Level 54
So by your response, you would agree with the assertions I have currently?
Level 61
I would agree with them. Bit verbose for my taste and breaking encapsulation, but if you like it then nothing major to worry about.
Level 54
Can you give further clarification of what you mean that its breaking encapsulation?
Level 61
Sure. $this->assertEquals(UserStatus::ACTIVE, $user->status) is breaking encapsulation. You are showing internal object state to the outside world and let them make decisions on it. What I would do is follow tell don't ask principle for it. So I would do $this->assertTrue($user->hasActiveStatus()) or $this->assertTrue($user->isActive()).
Also I wouldn't do $this->assertCount(1, $user->activations), I would use something as example above $this->assertTrue($user->hasActivations()).
$this->assertEquals($now->toDateTimeString(), $user->activations->first()->started_at->toDateTimeString()); is also to much to understand and comparing internal state outside of the object. Apply same logic as shown in two examples above.
Bonus:
$now = now(); Carbon::setTestNow($now); can be Carbon::setTestNow($now = now());
['started_at' => $startDate] can be ['started_at' => $startedAt ?? now()]
Based on the snippet, code below produces too many queries:
$this->activations()->updateOrCreate(['ended_at' => null], ['started_at' => $startDate]);
$this->updateStatusAndSave();
You should do your database interactions to a minimum. If you have 100 operations on the object you will have 100 queries just for that single object. You should do the transformations on the object state and just at the perimeter of the business layer just persist the changed state.
Take a look at the LoD principle.
List goes on, but that should be enough to get you started.
1 like
Level 54
That is A LOT of good information on your reply. Thank you so much.
Level 61
You are very welcome and thanks for listening. Usually people get defensive about their code and reject a helping hand for some reason. All best.
1 like
Level 54
@bugsysha One last thing I'm really wondering about my $this->assertEquals($now->toDateTimeString(), $user->activations->first()->started_at->toDateTimeString()); assertion. Wondering if I should bother with it in the first place.
What are your thoughts on that specific assertion?
Level 61
Not sure cause I don't fully understand the business logic behind it. I would try to cover as much as possible in my tests, if not everything. So my initial thought is that I would cover that also. Basically every state change so I do not have anything that is not covered and can cause problems down the road.
1 like
Level 54
Its funny. I wrote that assertion and now I can't even figure out why its important and how I could rewrite the assertion to make it more readable.
Level 61
I wrote that assertion and now I can't even figure out why its important
That tends to be the problem if you do not encapsulate things straight away.
how I could rewrite the assertion to make it more readable.
There are various ways, but it all depends on what does make sense for your codebase.
For start, you can remove ->toDateTimeString(). Also do not forget to call Carbon::setTestNow(); at the end to unfreeze it.
Also while we are talking about removal of code, I would remove tap(). In my opinion it just indents the code and does not provide real value here.
1 like
Please or to participate in this conversation.