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

[5.5] Add handleExceptions #20729

Merged
merged 1 commit into from
Aug 25, 2017
Merged

Conversation

sileence
Copy link
Contributor

@sileence sileence commented Aug 24, 2017

This method allows to deactivate the exception handling except for the given exceptions, that way, continuing with: #20724 I can have a test like:

        $this->handleValidationException()->post('profile', [
            'twitter' => '@{sileence}'
        ])
        ->assertSessionHasErrors('twitter');

And if an exception is thrown (other than a validation exception) then I will get the error in the console.

This way it is possible to get the best of both worlds?

Going further maybe I could add a handleExceptedException to throw an exception if the expected exception is not thrown (PHPUnit style)?

Or have some kind of wrapper like:

        $this->expectValidationErrors(['twitter'], function () {
            $this->post('profile', [
                'twitter' => '@{sileence}'
            ]);
        });

@sileence sileence changed the title Add handleExceptions [5.5] Add handleExceptions Aug 24, 2017
@taylorotwell
Copy link
Member

taylorotwell commented Aug 24, 2017

It feels like you are going to great lengths to simply avoid writing $response->assertStatus(200) which would have solved your problem in the first place?

@sileence
Copy link
Contributor Author

@taylorotwell well, 200, 422, 302.... So it can be confusing. But what I really don't like is having to go back to the test and type $this->withoutExceptionHandling() just to "uncover" the error. This idea or something similar could give more control over what exception is expected to be handled and allow faster debugging of unexpected exceptions.

@deleugpn
Copy link
Contributor

I use the exception logger to uncover the error.

@sileence
Copy link
Contributor Author

@deleugpn exactly, but that slows you down, and that's what Adam was trying to solve in the first place.

@sileence
Copy link
Contributor Author

Now with some tests you do need to have the exception handling turned on. What I'm proposing is: ok, let's turn it on but only for the specific exceptions I want the framework to handle for me. Throw everything else.

@deleugpn
Copy link
Contributor

deleugpn commented Aug 25, 2017

Personally I don't agree with your statement because this is somewhat related to preference. I prefer the logger over seeing a full stack trace in a command line interface.

That said, I can see you want a behavior that is slightly different than blunt withoutExceptionHandler (and the Framework doesn't have that now). This is a second subject: you'll have to carefully know what you're doing to be able to know which exceptions you want to bypass or not.

My suggestion: propose a PR where you only add the except parameter (which is more or less what you're missing) to the withoutExceptionHandler(). Make it optional and don't assume everyone wants to do that for ValidationException. Let each developer choose what they want.
You can even go one step further and declare a protected $handleExceptions = [] that we can override in the TestCase.

I think this could make everyone happy.

@sileence
Copy link
Contributor Author

@deleugpn I like the idea of the property, but that could also be implemented by the user.

The handleValidationException was a proof of concept. It could be removed. The important method is handleExceptions(array $exceptions).

@taylorotwell taylorotwell merged commit 1316741 into laravel:master Aug 25, 2017
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.

None yet

3 participants