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

Chain previous exception in AssertionFailedError #5069

Closed
wants to merge 5 commits into from

Conversation

mondrake
Copy link

@mondrake mondrake commented Oct 2, 2022

Fixes #5068

@sebastianbergmann
Copy link
Owner

Can you please add a test that fails without and passes with these changes? This probably needs to be an end-to-end PHPT test.

@mondrake
Copy link
Author

mondrake commented Oct 3, 2022

Will try. Do we have to consider traversing the exception chain up to the root one to get the trace at each re-throw, or just getting trace of the previous one is enough?

@mondrake mondrake marked this pull request as draft October 3, 2022 19:38
@mondrake
Copy link
Author

mondrake commented Oct 3, 2022

I cannot find a way to determine from within the exception whether the originating test was being run in isolation or not, so I added a parameter to the exception's constructor to indicate that context. Probably not a very suitable solution, some help would do.

@mondrake mondrake marked this pull request as ready for review October 3, 2022 20:14
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #5069 (81cfbde) into 9.5 (4f2a753) will decrease coverage by 0.02%.
The diff coverage is 64.70%.

@@             Coverage Diff              @@
##                9.5    #5069      +/-   ##
============================================
- Coverage     83.78%   83.75%   -0.03%     
- Complexity     4595     4602       +7     
============================================
  Files           273      273              
  Lines         11430    11447      +17     
============================================
+ Hits           9577     9588      +11     
- Misses         1853     1859       +6     
Impacted Files Coverage Δ
src/Framework/Exception/AssertionFailedError.php 68.42% <64.70%> (-31.58%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sebastianbergmann
Copy link
Owner

Please fix the issues identified by PHP-CS-Fixer and Psalm.

And may I also kindly ask you to properly configure your real name for Git's user.name setting?

@mondrake
Copy link
Author

mondrake commented Oct 5, 2022

I fixed the code style issues.

I will not disclose my real name in a public environment, sorry. I have committed with name + surname's initial, which is as far as I will go. Thanks for understanding.

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it as I do not believe this solution is the right one. Unfortunately, I currently do not have a better one.

I have created a branch that has a cleaned up version of your test case. This should serve as the foundation for future work on this issue.

In the meantime, I strongly recommend to change your implementation of onNotSuccessfulTest() to not rely on exception chaining. AFACIS, all that needs to be done is mapping the information from Mink's ExpectationException to PHPUnit's AssertionFailedError.

@mondrake mondrake deleted the dev-5068 branch October 6, 2022 10:36
@mondrake
Copy link
Author

mondrake commented Oct 6, 2022

Agree this was not viable. Biggest problem was impossibility to determine if a test was run in a process isolation context from within the exception.

Looking forward to a more sustainable solution. Let me know if I can help.

Thanks

@sebastianbergmann
Copy link
Owner

I have decided to not work on this and have deleted the branch.

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