Skip to content

phpt files with more than one line in title #17761

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

Closed
MasterZydra opened this issue Feb 11, 2025 · 11 comments · Fixed by #17763
Closed

phpt files with more than one line in title #17761

MasterZydra opened this issue Feb 11, 2025 · 11 comments · Fixed by #17763

Comments

@MasterZydra
Copy link
Contributor

Description

I implemented my own runner for the phpt files and noticed the following:

In the spec for the file format it says: (https://qa.php.net/phpt_details.php#test_section)
"Title of test as a single line short description."

But the following test have more than one line:

  • php-src\tests\classes\unset_properties.phpt
  • php-src\tests\lang\foreachLoop.010.phpt
  • php-src\tests\lang\func_get_arg.005.phpt
  • php-src\tests\output\stream_isatty_no_warning_on_cast.phpt

What is the best solution for this? Change the spec for phpt or fix the tests?

PHP Version

latest

Operating System

No response

@iluuu1994
Copy link
Member

Hi @MasterZydra. We should fix the spec, rather than the implementation.

@MasterZydra
Copy link
Contributor Author

MasterZydra commented Feb 11, 2025

Hi @iluuu1994,
Thanks for the fast reply. Should I open an issue/PR in the web-qa repo?

@iluuu1994
Copy link
Member

Yes, or submit a PR for this file: https://github.com/php/web-qa/blob/master/phpt_details.php. Note that there's also a PR to move this document to the main repo (this one): GH-15939. I'm not sure if @cmb69 still wants to proceed, but if he does, we should adopt the change there too.

@cmb69
Copy link
Member

cmb69 commented Feb 11, 2025

Please fix these overlong titles. That just doesn't make sense as implemented in run-tests.php at least, where the title is printed when running the test:

PASS Un-setting instance properties causes magic methods to be called when trying to access them from outside the magic
methods themselves. [C:\php-sdk\phpdev\vs17\x64\php-src\tests\classes\unset_properties.phpt]

Note that there is even the hard coded line break.

More descriptive information can be put in the --DESCRIPTION-- section.

I'm not sure if @cmb69 still wants to proceed

"want" vs. "have time/priority"

@iluuu1994
Copy link
Member

I don't mind also adjusting the tests, but breaking tests (remember that the same script is used for extensions) seems a bit harsh.

@cmb69
Copy link
Member

cmb69 commented Feb 11, 2025

Well, I would fix the tests (no changes to run-tests.php), and leave the spec as is. This doesn't break anything.

@iluuu1994
Copy link
Member

I reckon OP opened this issue because they're attempting to make their own implementation comply to the spec. So, adjusting the test alone might not answer their question about how it should behave.

@cmb69
Copy link
Member

cmb69 commented Feb 11, 2025

I suggest to apply Postel's law: "be conservative in what you do, be liberal in what you accept from others"

I.e. a runner should (ideally) deal with multiline titles, but the spec should still be strict.

@MasterZydra
Copy link
Contributor Author

I suggest to apply Postel's law: "be conservative in what you do, be liberal in what you accept from others"

I.e. a runner should (ideally) deal with multiline titles, but the spec should still be strict.

I'm fine with that solution.

@cmb69 Should I create a PR to fix the tests title?

@cmb69
Copy link
Member

cmb69 commented Feb 11, 2025

Should I create a PR to fix the tests title?

That would be nice!

@MasterZydra
Copy link
Contributor Author

MasterZydra commented Feb 11, 2025

Thanks you two for the quick solution.
PR created: #17763

@cmb69 cmb69 linked a pull request Feb 13, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants