Skip to content

Fix imprecise %s placeholders in phpt test expectations#22065

Open
jorgsowa wants to merge 1 commit into
php:masterfrom
jorgsowa:fix/phpt-imprecise-placeholders
Open

Fix imprecise %s placeholders in phpt test expectations#22065
jorgsowa wants to merge 1 commit into
php:masterfrom
jorgsowa:fix/phpt-imprecise-placeholders

Conversation

@jorgsowa
Copy link
Copy Markdown
Contributor

In --EXPECTF-- sections, %s placeholders were being used to match more than just file paths, inadvertently absorbing line number information and making the assertions weaker than intended.

@bwoebi
Copy link
Copy Markdown
Member

bwoebi commented May 16, 2026

This is not quite inadvertent, but it's sort of pointless to test the very same thing thousands of times instead just a hundred times. It's not quite the point of the tests to test the formatting of the error string.

There are enough tests in Zend/ which test error string formatting.

@jorgsowa jorgsowa force-pushed the fix/phpt-imprecise-placeholders branch from 22e88c1 to 9dc95c4 Compare May 16, 2026 19:43
@jorgsowa
Copy link
Copy Markdown
Contributor Author

It's not testing the formatting of the error string, but asserting the exact error message instead. The placeholder is used to hide the file path and line number (I'm not a fan of this approach, but it's a convention in php-src, so it was included), as advised in the documentation:

Alternatively you can use --EXPECTF-- and check for the message by replacing the path of the source of the message with %s and the line number with %d.

https://github.com/php/php-src/blob/master/docs/source/miscellaneous/writing-tests.rst#error-reporting-in-tests

If those assertions are pointless, then where is the boundary for such assertions? What part of the error message should we test and what shouldn't we? In which directories should there be a full error message, and in which should there be a wider placeholder? I think answering those questions is more difficult than applying the same standard to all tests.

@jorgsowa jorgsowa marked this pull request as ready for review May 16, 2026 19:56
@kamil-tekiela
Copy link
Copy Markdown
Member

We want to test the error message. For warnings, notices and deprecations, it's the part before "in ...". For errors and exceptions, it's the value of getMessage().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants