search

php / php-src

The PHP Interpreter
https://www.php.net
Other
38.17k stars 7.75k forks source link

Organize `Zend/tests/*.phpt` tests into subdirectories for easier navigation #15631

Open DanielEScherzer opened 2 months ago

DanielEScherzer commented 2 months ago

Description

I wanted to take a look at any of the enum-related tests, but on GitHub navigating to https://github.com/php/php-src/tree/master/Zend/tests shows

Sorry, we had to truncate this directory to 1,000 files. 1,534 entries were omitted from the list. Latest commit info may be omitted.

I propose that the tests that are directly in the top-level Zend/tests/*.phpt be organized a bit with sub directories so that they can be navigated more easily, both on GitHub and in downloaded copies of PHP. For an easy example, there are 21 different tests for creating class aliases, class_alias_001.phpt through class_alias_021.phpt - these could be grouped together in their own directory (probably just named class_alias).

I'm happy to send the patches to work on this reorganization if there is buy-in, but figured I should ask before I started the work. Since this doesn't affect the actual execution of PHP I didn't think it was applicable to propose on the internals list, so adding it here.

PS: this is tagged as a feature but only because that was the most applicable of the GitHub issue forms, but it isn't really a feature

iluuu1994 commented 2 months ago

Hi @DanielEScherzer. That will be a pain in the butt when merging bug fixes upward. Not sure I'd be in favor.

DanielEScherzer commented 2 months ago

Hi @DanielEScherzer. That will be a pain in the butt when merging bug fixes upward.

I hadn't thought about that. But, looking through all 8.2 commits since the start of the month, I only count 6 commits that modify or delete existing test files (rather than adding new files, which shouldn't cause problems) and all 6 are for extensions (7878a2c322c7f394f3900a59e36a12002fdc55a5, 1b52ecd78ad1a211a4a9db65975df34d2539125b, 4d71580e005e8711c55130d3080960e81a150922, 28290655e864949ed0c08cad77240fb0956ab6e9, 6713d516319074bc2481b8e7db82b10841c230f7, 11fbe8801bb936c7011b4cefb3bea89380bf18e7). In fact, I had to go all the way back to your May 6 commit f8d1864bbbfbdc63d1b8e4e58934f8d8668f55bf to find an 8.2 commit that affected existing Zend test files, and the most recent for 8.3 that I could find was also from you that day, 5aa5080ea7fa54e19f1fd11b68868564881512a8

My understanding from looking through the git history is that normally bug fixes only result in adding new tests, since changing existing tests suggests that the fix also modifies some stable behavior?

iluuu1994 commented 2 months ago

rather than adding new files, which shouldn't cause problems

Not problems, per-se. But if the idea is to split the tests, then they probably no longer belong in root and will have to be moved again.

DanielEScherzer commented 2 months ago

rather than adding new files, which shouldn't cause problems

Not problems, per-se. But if the idea is to split the tests, then they probably no longer belong in root and will have to be moved again.

My proposal was just to split the tests enough to be navigable on GitHub, I'm not suggesting to create a strict hierarchy - we would still most likely end up with a bunch of tests in the top level. How about I send a demo patch to show what I'm thinking?

cmb69 commented 2 months ago

Modifying the directory structure is always a bit of an issue, not only for merging, but also for git log and git blame etc. However, if we have proper git renames, it's probably not that bad.

Maybe we should start moving all bug.phpt and bug directories to a bug/ directory, and do the same for gh*. Especially the former would reduce the number of toplevel dirents considerably. And if occassionally a file is not moved into the right directory when merging up, that's not a big deal anyway.

DanielEScherzer commented 2 months ago

Just wanting to note that there is https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view to allow ignoring some commits via git blame, that might satisfy the concerns

I sent #15638 as a demonstration of what I was thinking - while I agree that having a top-level bug/ would probably help, that is a case where there might be more conflicts merging up, so I didn't want to start there

DanielEScherzer commented 3 weeks ago

@cmb69 I see that you linked my PR as closing this issue - it was merely intended as a first step, https://github.com/php/php-src/tree/master/Zend/tests still reports

Sorry, we had to truncate this directory to 1,000 files. 1,370 entries were omitted from the list. Latest commit info may be omitted.

Would you mind re-opening this issue? I don't seem to have the rights to do so. After #16423 merges I'll go through the whole set of bug*.phpt and gh*.phpt tests and try and organize those into existing sub directories, since that seems to be where the vast majority of the remaining tests are concentrated