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

[11.x] Add assertThrown() #54046

Draft
wants to merge 1 commit into
base: 11.x
Choose a base branch
from
Draft

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Dec 31, 2024

This adds a testing helper for asserting a closure throws an exception/error, ensures the type, and then returns the exception. This is similar to how JUnit's assertThrows works.

The big difference from Laravel's assertThrows() is that you can make expectations against the exception.

Code examples

This is taken from tests/Auth/AuthAccessResponseTest.php.

The way its written currently

public function testAuthorizeMethodThrowsAuthorizationExceptionWhenResponseDenied_original()
{
    $response = Response::deny('Some message.', 'some_code');

    try {
        $response->authorize();
    } catch (AuthorizationException $e) {
        $this->assertSame('Some message.', $e->getMessage());
        $this->assertSame('some_code', $e->getCode());
        $this->assertEquals($response, $e->response());
    }
}

Rewritten to use the assertThrows

public function testAuthorizeMethodThrowsAuthorizationExceptionWhenResponseDenied_assertThrows()
{
    $response = Response::deny('Some message.', 'some_code');

    $this->assertThrows(
        fn() => $response->authorize(),
        function(AuthorizationException $e) use ($response) {
            $this->assertSame('some_code', $e->getCode());
            $this->assertEquals($response, $e->response());

            return true;
        },
        "Some message"
    );
}

Using the newly added method

public function testAuthorizeMethodThrowsAuthorizationExceptionWhenResponseDenied_assertThrown()
{
    $response = Response::deny('Some message.', 'some_code');

    $e = $this->assertThrown(fn () => $response->authorize(), AuthorizationException::class);

    $this->assertSame('Some message.', $e->getMessage());
    $this->assertSame('some_code', $e->getCode());
    $this->assertEquals($response, $e->response());
}

My belief is:
The first way (try-catch and perform assertions in the catch block) is most common and works fine.

The second way (assertThrows()) is cumbersome to write, requires you to remember to add return true to the end, and the nested closure makes it difficult to parse. If you don't add return true to the end, you can just convert the second closure into a series of boolean statements, but then you can't really leverage the PHPUnit assertion methods which is a big loss.

The third way (assertThrown()) breaks down a layer of indentation, doesn't require adding any use ($otherVariablesFromOuterScope), nor does it have the gotcha of remembering to return true.


I do not love the name of this method, but couldn't think of anything better. I have added this as a trait in a work project, and figured I would see if it was of interest to the Laravel community at large.

@shamimulalam
Copy link
Contributor

Seems very good method to have :)

@ziadoz
Copy link
Contributor

ziadoz commented Jan 1, 2025

If it's of any help, PHPUnit has assertions for exceptions:

public function test_authorize(): void
{
    $this->expectException(AuthorizationException::class);
    $this->expectExceptionMessage('Some message');

    Response::deny('Some message', 'some_code')->authorize();
}

@cosmastech
Copy link
Contributor Author

If it's of any help, PHPUnit has assertions for exceptions:

public function test_authorize(): void
{
    $this->expectException(AuthorizationException::class);
    $this->expectExceptionMessage('Some message');

    Response::deny('Some message', 'some_code')->authorize();
}

That's a good point! I think what this approach still lacks is being able to perform additional assertions against the exception. For instance:

class PaymentFailed extends RuntimeException
{
    public PaymentMethodEnum $method;
    public User $receivingUser;

    public function setPaymentMethod(PaymentMethodEnum $method): void
    {
        $this->method = $method;
    }

    public function setReceivingUser(User $user): void
    {
        $this->receivingUser = $user;
    }
}

I may not render both of those pieces of information in the exception message, but I may want to perform assertions against them.

@crynobone
Copy link
Member

Doesn't this already solved with #50704

@cosmastech
Copy link
Contributor Author

Doesn't this already solved with #50704

I did not know this existed. Thanks for mentioning it and nice work @nunomaduro!

  1. This would be unavailable in tests which don't boot the application (ie, extending from PHPUnit's TestCase)
  2. It won't work for a case like this:
public function test_uniqueEmail() {
    Exceptions::fake();

    // Given
    Org::factory()->create(['id' => 12]);
    $userFactory = fn () => User::factory()->create(['email' => '[email protected]', 'org_id' => 12]);
    // And the user exists
    $userFactory();

    // When we attempt to create a user with the same email address
    $userFactory();

    // Then
    Exceptions::assertReported(function(UniqueConstraintViolationException $e) {
    self::assertStringContainsString(
            "Duplicate entry '[email protected]' for key 'users.users_email_org_id_unique'",
            $exception->getMessage()
        );
        return true;
    });
}

☝️ this could be solved with PHPUnit's expectException/expectExceptionMessage of course.

I'm not sure if I'm doing something wrong, but it doesn't seem to work here either?

public function test_ModelNotFound()
{
    // Given a route
    Route::get('/some-example', function () {
        return User::findOrFail(1);
    });

    // And we are faking exception handling
    Exceptions::fake([ModelNotFoundException::class]);
    // $this->withoutExceptionHandling();   <-- also tried this but it didn't seem to change anything

    // When we request the endpoint
    $response = $this->get('/some-example');

    // Then the response is not found
    $response->assertNotFound();

    // And the exception looks like this
    Exceptions::assertReported(function (ModelNotFoundException $e) {
        self::assertStringContainsString('No query results for model [App\Models\User] 1', $e->getMessage());

        return true;
    });
}
  1. It requires knowing which exceptions are set on Handler::$dontReportInternal (I think?) as well as which exceptions your application has indicated to not report.
  2. It requires returning true at the end of the expectation closure (which isn't indicated in the docblock)
  3. It has the problems of being a closure (additional indentation, more cumbersome to read/write data to the outside scope)

Obviously anyone is free to add their own AssertsExceptionsTrait, this isn't a bug, but a quality of life improvement.

@taylorotwell taylorotwell marked this pull request as draft January 2, 2025 22:52
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.

4 participants