Skip to content

Patch run tests from git work tree #18286

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hakre
Copy link
Contributor

@hakre hakre commented Apr 9, 2025

I still had a leftover when looking into the dom nodelist iteration hickup @nielsdos fixed in #18004 : sorting the list of skipped extensions in the ‎run-tests.php report. details in the commit message.

And then while rebasing today, a generate test .data file caught my eye and I added the fix. details in the commit message.

/cc @cmb69

hakre added 2 commits April 9, 2025 11:01
Sort flags are the same as for $exts_to_test (flags: SORT_REGULAR) in
main():730.

There was also a slight chance to branch into get_summary() with
(show_ext_summary: true) and count(NULL) use:  Given the call of
find_files() in main() on line 635, when there are no extensions to
skip, then the global variable $exts_skipped is not converted to an
array ($global[] = ...), that errors on count() then.

Therefore, we cast the global variable $exts_skipped to array before
sorting and counting.
The cve-2014-3538-mb.phpt test similar to cve-2014-3538.phpt removes
the .data file in --CLEAN-- by unlink().

However, in 3d3f11e ("Fixed the UTF-8 and long path support in the
streams on Windows.", 2016-06-20) the temporary fixture file has been
committed.

As the file is generated this is counter-productive.  Furthermore,
when running the tests under version control given a build from checkout,
then the VCS marks the file as deleted.

Fix is to remove the file from version control:

    $ git rm --cached ext/fileinfo/tests/cve-2014-3538私はガラスを食べられます.data

To prevent similar mistakes, ignore those test .data files in git with the
binary attribute macro to levitate the known issue in diff fetching tools.
Comment on lines +34 to +36

# Test data files
**/tests/**/*.data binary
Copy link
Member

Choose a reason for hiding this comment

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

Should probably not be in both .gitignore and .gitattributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? @TimWolla

Copy link
Member

Choose a reason for hiding this comment

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

.gitattributes applies to committed files, where-as .gitignore prevents files from being committed. So either of those is redundant (or should be more specific, or should have exclusions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not true, .gitattributes are similar as .gitignores for the worktree as well.

And actually, that's the reason I put it there because the binary macro has -diff and diffing caused an issue resolving this. Therefore I added it.

Nevertheless, if there are still doubts about it, I can (happily!) split this PR and move it out. No need to rush the by-catch IMHO. Let me know and thanks for your feedback so far!

@@ -3012,6 +3012,9 @@ function get_summary(bool $show_ext_summary): string
$summary = '';

if ($show_ext_summary) {
static $exts_skipped_sorted;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you cache this?

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