haakym's avatar

Refactoring a test for user registration

I've written a test to check a user can register. The main outcomes of the registration are that a new user is in the database and is sent an email requesting they verify their registration. The test seemed a bit long to me and I thought I'd get some second opinions here on the forum with how I might refactor it. Here's the code:

/** @test */
public function a_user_can_register()
{
    $this->seedDatabase(); // seeds the database with roles which are assigned on user creation

    Mail::fake();

    NoCaptcha::shouldReceive('verifyResponse')
        ->once()
        ->andReturn(true);

    $response = $this->json('POST', '/register', [
        'name' => 'John Doe',
        'email' => '[email protected]',
        'contact_number' => '12345678',
        'account_type' => 'personal',
        'password' => '123456',
        'password_confirmation' => '123456',
        'g-recaptcha-response' => '1',
    ]);

    $response->assertRedirect('/auth/verification/sent');

    $this->assertDatabaseHas('users', [
        'name' => 'John Doe',
        'email' => '[email protected]',
        'contact_number' => '12345678',
        'verified' => 0,
    ]);

    $user = User::whereEmail('[email protected]')->first();

    Mail::assertSent(VerificationTokenGenerated::class, function ($mail) use ($user) {
        return $mail->hasTo($user->email);
    });
}

Would I be better of splitting each assertion into its own method and moving the actions that are used prior to the assertions into private methods or and the constructor?

0 likes
3 replies
Talinon's avatar

One thing I could suggest is instead of manually building the user array, just make (not create) a user factory and pass with ->toArray() into your json call. I also don't see a need for the temporary $response variable if you just chain the redirection assertion.

$this->json('POST', '/register', $user->toArray())
        ->assertRedirect('/auth/verification/sent');

Then when you assert it against the database, you can reference the user attributes, again:

$this->assertDatabaseHas('users', [
        'name' => $user->name,
        'email' => $user->email,
        'contact_number' => $user->contact_number,
        'verified' => 0,
    ]);

If you wanted to clean it up further, you could refactor the assertDatabaseHas() into its own method, which accepts your User class and runs the assertions.

Those simple refactors would clean it up a bit.

1 like
haakym's avatar

@Talinon Thanks so much for your suggestions!

Annoyingly I can't reduce the request body so much because half of it is required for the form validation rules and isn't part of the user's attributes (of course I didn't expect you to know that or anything).

In any case, it was definitely a move in the right direction as I have swapped out some unnecessary code including the eloquent call by utilising the $user that I created (or make()'d) with the factory.

Right now I've got it looking like this...

public function setUp()
{
    parent::setUp();

    $this->seedDatabase(); // seeds the database with roles which are assigned on user creation
}

/** @test */
public function a_user_can_register()
{
    Mail::fake();

    NoCaptcha::shouldReceive('verifyResponse')
        ->once()
        ->andReturn(true);

    $user = factory(App\Models\User::class)->states('non-verified')->make();

    $response = $this->json('POST', '/register', 
        array_merge($user->toArray(), [
            'account_type' => 'personal',
            'password' => '123456',
            'password_confirmation' => '123456',
            'g-recaptcha-response' => '1',
        ])
    );

    $response->assertRedirect('/auth/verification/sent');

    $this->assertDatabaseHas('users', 
        array_merge($user->toArray(), [
            'verified' => 0,
        ])
    );

    Mail::assertSent(VerificationTokenGenerated::class, function ($mail) use ($user) {
        return $mail->hasTo($user->email);
    });
}

I'm still playing with the idea of abstracting the arrange and act parts into a single method then separating the assertions into their own methods. I'm trying to reach a place where it seems more readable which is why I've kept the act step (the POST request) assigned to the $response variable as I find it easier to read as I've already got the array merge going on inside the request body which honestly I don't like that much but I'll just leave it as it is for now and see if I get any fresh perspective later. Perhaps I'll separate the request body build up into its own variable so it's easier to reason about? Something like ...

$data = array_merge($user->toArray(), [
        'account_type' => 'personal',
        'password' => '123456',
        'password_confirmation' => '123456',
        'g-recaptcha-response' => '1',
    ]);

$response = $this->json('POST', '/register', $data);
mdecooman's avatar

Hi @haakym

Instead of using the assertDatabaseHas (I even didn't know about it), I use as follow (I use the trait RefreshDatabase):

Mail::assertQueued(ConfirmYourEmail::class);
$user = User::whereEmail('[email protected]')->first();
$this->assertFalse($user->confirmed);

The in the model I also use a casts to make sure this attribute is interpreted as a boolean

protected $casts = [
        'confirmed' => 'boolean'
    ];

Within the same test class you may want to separate your tests. 1 to check a_user_can_register, a second like a_user_receives_an_email_asking_confirmation_upon_registration, etc.

Don't focus to much on a single class test, better to come back later with a fresh mind if even necessary. I used to procrastinate too much on some of my tests, avoiding to move forward with the features I needed to develop.

Hope it helps

Best

1 like

Please or to participate in this conversation.