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

Tests with Pest PHP #2

Merged
merged 7 commits into from
Jan 25, 2022
Merged

Tests with Pest PHP #2

merged 7 commits into from
Jan 25, 2022

Conversation

dansysanalyst
Copy link
Contributor

Hello Mostafa,

Congratulations on your project and your journey. Very good project presentation!

I got to know about you in Laravel Daily and wanted to contribute with your project.

There are some interesting changes on this Pull Request: #1. Do you know how what Pull Requests are and how to manage them? If not, just comment!

My Pull Request is about automated tests.

Tests can be a controversial topic, people have their own opinion about it. Me, myself, I wish I was introduced to testing in my first day, when I started as a developer.

I believe tests save time, headache, improve quality and makes maintenance easier.

It is virtually impossible for you to test different things every time you make a change in your application. How do you know if your new code did not break some other feature? We do not have time to test every single page or rule of our applications when new code is added.

For this, we have automated tests.

My approach is to test whatever is critical to my application. For example, if I have a blog system, I want to make sure a user can create, update, view, and delete a post. I want to guarantee that guests cannot create posts or view admin pages.

These are the tests I would start to write, and I did write some of

These are the tests currently running in your application. I have created the PostTest and RouteTest.

My test framework of choice is Pest PHP.

To run tests, you can type ./vendor/bin/pest in your Terminal (or composer test).

CleanShot 2022-01-23 at 13 06 49@2x

Green tests mean everything is okay, yellow is skipped and red is a failed test.

For example,

This test visits your home page and verifies that it loads.
(Status 200 means OK, page loads)

test('Home page loads', function () {
    //Requests the page
    $response = $this->get(route('home'));

    //Page could be loaded
    $response->assertStatus(200);
});

According to your rule, only the user MooseS94 can access the Dashboard.
As mentioned in the PR #1, this logic could/should be modified, but let's say this is the final objective.
I wrote a test for that:

test('Dashboard: Random user can NOT access dashboard', function () {

    // Create an user for this test
    $user = User::factory()->create(['username' => 'random1234']);

    $response = $this->actingAs($user)
        ->get(route('dashboard'));
    
    //403 Forbidden access
    $response->assertStatus(403);
});

In the test above, we verified that a user random1234 will not be able to access the Dashboard.

And in the next test, we verified that MooseS94 can do it.

test('Dashboard: MooseS94 can access dashboard', function () {

    // Create MooseS94 for this test
    $user = User::factory()->create(['username' => 'MooseS94']);

    $response = $this->actingAs($user)
        ->get(route('dashboard'));

    $response->assertStatus(200);
});

Now, how can you make sure these tests are working?

Just for an exercise, modify your MustBeAdmin middleware to:

    public function handle(Request $request, Closure $next)
    {

       if (auth()->user()?->username != 'Dan1234') {

            abort(Response::HTTP_FORBIDDEN);
        }

        return $next($request);
    }

Now, only dansysanalyst can access the Dashboard, but the test is written to verify if MooseS94 can do it.

So, it will make your test fail:

CleanShot 2022-01-23 at 13 23 54@2x

You can follow this playlist to see a bit more about tests:

https://www.youtube.com/watch?v=gTU-y6HlmzU&list=PLNXrjfSe7qHncCyQYOqJBTsTbYPotMaZ8&ab_channel=MichaelDyrynda

I wish you the all best,

Greetings

Dan

@MooseSaeed
Copy link
Owner

Hello Dan,

Thank you so much for your great efforts and wonderful explanation of testing.

I sort of understand what a pull request is and I've seen #1 request but I thought that I'll do the changes myself to practice more and I actually did.

When it comes to MooseS94 is the only user who can see the dashboard that's because I considered Laravel Breeze's default dashboard is an Admin Dashboard because I won't allow guests to register or login, I wanted only my friend to be able to login and make changes.

In regards of testing. This is a great introduction and I will have a closer look on this pull request along with the playlist your share with me as soon as I continue working on this project very soon.

Thank you again for your explanation and for taking the time to benefit a complete stranger :)

@dansysanalyst
Copy link
Contributor Author

Hi Mostafa,

This is an opinion, not a rule or advice: I think it's nice to accept the pull request and then work around it, based on it.
I view it as a way to thank the contributor :)

About the user, MooseS94 I would strongly suggest making a small tweak on the users table:

  • add a field is_admin to the table: $table->boolean('is_admin')->default(false); and by default, it is false.
  • Do not add is_admin to your fillable
  • Change your middleware to something like if (auth()->user()?->is_admin == false)
  • You can now create user A, B or C and control their access to these pages.

There are more complex solutions, but I think this is simple and close enough to a real-world scenario.

Sharing knowledge is great because it forces us to learn again, re-think

Strangers or not, we are in the same community. We all grow together :)

@MooseSaeed MooseSaeed merged commit f8f8273 into MooseSaeed:master Jan 25, 2022
@MooseSaeed
Copy link
Owner

Great opinion :D Will work on it.

Thanks again 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants