-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Output buffer - fix 5851 #5852
base: main
Are you sure you want to change the base?
Output buffer - fix 5851 #5852
Conversation
Test code or tested code closed output buffers other than its own | ||
|
||
%sIssue5342Test.php:%i | ||
2) PHPUnit\TestFixture\Issue5342Test::testFailure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this escalation from risky test to test failure intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I didn't plan that initially, but as I created the PR, I realized that more likely than not these ob
issues lead to unexpected and non-trustworthy test results in many cases. Keeping them "risky" would not convey the severity of the issue here in my opinion.
I did keep "risky" in some cases (as you can see) though, were the behavior is usually not much different than if there were no ob issue at all.
Due to the nature of the buffer, the resulting logic/code is unfortunately a bit ugly, so feedback is very welcome.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5852 +/- ##
============================================
- Coverage 92.32% 92.22% -0.10%
- Complexity 6555 6597 +42
============================================
Files 699 699
Lines 19775 19869 +94
============================================
+ Hits 18257 18324 +67
- Misses 1518 1545 +27 ☔ View full report in Codecov by Sentry. |
private function stopOutputBuffering(): bool | ||
{ | ||
$bufferingLevel = ob_get_level(); | ||
$bufferingLevel = ob_get_level(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, it is a bugfix, so it must target the lowest supported and problematic branch.
Also, it seems this PR fixes several ob issues. I would like to see a separate commit for each issue ("Fix ob is not cleanable", ...) so each can be reviewed individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, it is a bugfix, so it must target the lowest supported and problematic branch.
Could you give me a quick hint, since this is my first PR for phpunit - which branch would that be? Rebasing/Rebranching from that and implementing it on that branch is sufficient or is there anything else?
Also, it seems this PR fixes several ob issues. I would like to see a separate commit for each issue ("Fix ob is not cleanable", ...) so each can be reviewed individually.
Practically this isn't possible except for a single case (the error added for ob_flush/ob_clean), since all changes are needed for any of these cases to work correctly. Separating it would make it more confusing than it already is - due to the nature of output buffering.
In case you insist, could you please tell me the exact commits/how you want it separated, then I can see if it's possible or tell you exactly why it's not possible.
Fix #5851