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

[9.x] Improve test failure output #43943

Merged
merged 7 commits into from
Sep 15, 2022
Merged

[9.x] Improve test failure output #43943

merged 7 commits into from
Sep 15, 2022

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Aug 31, 2022

In #38025 and #38046, we improved the test output for the assertStatus methods to display any exceptions or errors that may have caused the status assertion to fail.

These improvements almost removed the need to call withoutExceptionHandling. Almost...

This PR expands on the previous work to display exceptions or errors on any failed assertion, not just status assertions, and not even just TestResponse assertions.

A common example is when asserting Inertia redirects, and an unexpected validation error occurs. The current output doesn't give any clues that the redirect assertion failure was due to a validation error:

image

Under the hood, this calls assertStatus and assertLocation, but it's only the assertLocation that fails, which doesn't currently display exceptions or errors.

With the changes in this PR, the error now looks like this:

image

Note Unfortunately, it doesn't seem possible to inject the exception/error after the ComparisonFailure diff, so the output is not 100% what I'd want.

The existing assertStatus behaviour is now handled by this new logic, so there's no duplication of code. There is a minor change in output because the extra content is now appended after PHPUnit's raw assertion error instead of before it:

Before:
image

After:
image

We'll now also see this extra context even if we don't assert against the TestResponse. For example, imagine this test where we expect a user record to be created:

$this->post('/users', []);

$this->assertSame(1, User::count());

If a validation error or exception occurred during the last request, we'll get the extra context on PHPUnit's built-in assertions like assertSame:

image

The only scenario I can think of where this feature is undesirable is the assertJson assertion. If a validation error occurs, the assertJson assertion will already show the full response body. To prevent duplicate output, I've ensured that we don't append the JSON error response if the existing message already includes it.

If the user makes multiple requests in one test, only errors/exceptions from the latest request will be displayed, hopefully preventing red herrings.

The extra context will only be displayed when an assertion fails, so you won't see any output if you expect an error or exception as long as your assertions pass.

Developers may opt-out of this feature by overriding the onNotSuccessfulTest method in their base test case to throw the given exception like PHPUnit does by default.

@jessarcher jessarcher force-pushed the improve-test-errors branch 2 times, most recently from aa14cc5 to 288583b Compare August 31, 2022 08:14
Copy link
Member

@jbrooksuk jbrooksuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jessarcher you may have put EOF where it is for indentation, so feel free to ignore this.

@jessarcher jessarcher marked this pull request as ready for review September 5, 2022 04:09
@jessarcher
Copy link
Member Author

@nunomaduro I'd appreciate your thoughts on this one.

Do you see any issues hooking into onNotSuccessfulTest and appending to the message property on ExpectationFailedException exceptions?

Would this feature also be beneficial for Pest or can you think of any issues it might cause?

@jessarcher jessarcher changed the title [9.x] Append exceptions and errors on all test failures [9.x] Improve test faliure output Sep 6, 2022
@jessarcher jessarcher changed the title [9.x] Improve test faliure output [9.x] Improve test failure output Sep 6, 2022
@taylorotwell taylorotwell marked this pull request as draft September 6, 2022 14:54
@taylorotwell
Copy link
Member

@jessarcher marking as draft until we get feedback from @nunomaduro

: implode(PHP_EOL, Arr::flatten($errors));

// JSON error messages may already contain the errors, so we shouldn't duplicate them.
if (str_contains($exception->getMessage(), $errors)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this works? Asking because it seems we change the format of the JSON string by using the JSON_PRETTY_PRINT flag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - the existing output in the exception is also pretty printed

@nunomaduro
Copy link
Member

@jessarcher I've rebased, added minor changes, and made a few comments. Generally, this pull request seems OK to me. In addition, I've tested this pull request against Vapor test suite - more than 600 test cases - and seems to be all working.

@nunomaduro nunomaduro removed their assignment Sep 13, 2022
@jessarcher jessarcher marked this pull request as ready for review September 13, 2022 23:34
@taylorotwell taylorotwell merged commit b7e096a into 9.x Sep 15, 2022
@taylorotwell taylorotwell deleted the improve-test-errors branch September 15, 2022 19:03
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