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

Add expectErrorLog() for expecting error_log() output #6118

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Feb 5, 2025

summary:

  • when a test which invokes error_log() without a call to expectsErrorLog(), the output is directly rendered in-between the summary (as tested in Regression test for error_log() #6127)
  • when a test which invokes error_log() with a call to expectsErrorLog(), no more output is rendered in-between the test summary printing. when no error_log() is invoked the test is considered failed.

backstory:

while looking into #2155 (comment) I realized PHPunit is already somehow capturing the error_log() output.

I think this means we don't need a workaround like #2155 (comment) in phpunit itself, but just a API which allows us to make assertions on the already captured output.

DX wise I think there is also some room for improvement since the error_log'ged string just shows up in the test-debug output without any indication where this string is coming from (the test-author needs to look into the code beeing tested to realize that this string originates from a error_log) or how to make assertions on it.

With this PR I am just sending a test so we can see the status quo and discuss where to move from here

closes #2155

@staabm
Copy link
Contributor Author

staabm commented Feb 7, 2025

I tried debugging it a bit more to find out how phpunit knows what the unit-under-test passed to error_log in the PRs test-case.

PHPUnit Started (PHPUnit 12.0-gd39bf28f4 using PHP 8.3.16 (cli) on Darwin)
Test Runner Configured
Bootstrap Finished (/Users/m.staab/Documents/GitHub/phpunit/tests/bootstrap.php)
Event Facade Sealed
Test Suite Loaded (1 test)
Test Runner Started
Test Suite Sorted
Test Runner Execution Started (1 test)
Test Suite Started (PHPUnit\TestFixture\Issue2155\Issue2155Test, 1 test)
Test Preparation Started (PHPUnit\TestFixture\Issue2155\Issue2155Test::testOne)
Test Prepared (PHPUnit\TestFixture\Issue2155\Issue2155Test::testOne)
logged a side effect
Test Passed (PHPUnit\TestFixture\Issue2155\Issue2155Test::testOne)
Test Finished (PHPUnit\TestFixture\Issue2155\Issue2155Test::testOne)
Test Suite Finished (PHPUnit\TestFixture\Issue2155\Issue2155Test, 1 test)
Test Runner Execution Finished
Test Runner Finished
PHPUnit Finished (Shell Exit Code: 0)

I can see the string also be mentioned in the event log stream of --debug.
@localheinz could you give me a hint how I can find out where this logged a side effect string is coming from?

@staabm
Copy link
Contributor Author

staabm commented Feb 7, 2025

ohh I think I understand the problem now. error_log is just writing into stderr. its not a event happening and beeing rendered by the event system

@clxmstaab clxmstaab force-pushed the bug2155 branch 2 times, most recently from 1507fae to fa8961d Compare February 7, 2025 13:11
@staabm staabm changed the title Test error_log() in tests Allow assertions on error_log() output Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.09%. Comparing base (fb459a7) to head (18a89b4).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6118   +/-   ##
=========================================
  Coverage     95.09%   95.09%           
- Complexity     6739     6742    +3     
=========================================
  Files           724      724           
  Lines         21267    21278   +11     
=========================================
+ Hits          20223    20234   +11     
  Misses         1044     1044           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staabm staabm marked this pull request as ready for review February 7, 2025 13:28
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/assertion Issues related to assertions and expectations labels Feb 7, 2025
@sebastianbergmann sebastianbergmann added this to the PHPUnit 12.1 milestone Feb 7, 2025
@staabm staabm marked this pull request as draft February 14, 2025 10:22
@staabm staabm changed the title Allow assertions on error_log() output Support expectsErrorLog() Feb 14, 2025
@staabm staabm force-pushed the bug2155 branch 2 times, most recently from a078ae1 to 5b5c7ee Compare February 15, 2025 08:09
@staabm staabm marked this pull request as ready for review February 15, 2025 08:39
@staabm
Copy link
Contributor Author

staabm commented Feb 15, 2025

I think we are in better shape now (put a new summary into the PR description)

@@ -1007,6 +1014,11 @@ final protected function expectOutputString(string $expectedString): void
$this->outputExpectedString = $expectedString;
}

final protected function expectsErrorLog(): void

Choose a reason for hiding this comment

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

This should be expectErrorLog() to be consistent with expectOutputString, for example.

@sebastianbergmann sebastianbergmann changed the title Support expectsErrorLog() Add expectErrorLog() for expecting error_log() output Feb 17, 2025
@@ -0,0 +1,31 @@
--TEST--

Choose a reason for hiding this comment

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

expectErrorLog() is a new feature. Therefore, we should not put its end-to-end tests into the regression directory. The generic would be acceptable for now.

@staabm
Copy link
Contributor Author

staabm commented Feb 17, 2025

great catch, fixed it. thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expectOutputString and error_log
2 participants