Trim trailing whitespace in tests

[carusogabriel]

Following #2995 and a reworked version of #3039.

Opened to run against CIs and see the affected tests.

[carusogabriel]

@petk Remember a while ago this comment #3468 (comment)? Give this patch a git show --ignore-space-change locally and you'll see what I meant :)

Do you have any idea how can we fix those tests? Also, there're some tests in .gitattributes with the -crlf attribute. Should we prepare some patch to fix those as well?

[petk]

Of course... I remember. I'm just not that fast with all the file fixing scripts... :D

Case is that those files are binary files. For example:

file -k ext/standard/tests/array/each.phpt

shows ext/standard/tests/array/each.phpt: data... Similarly for example how it is handled a zip file. Now, I'm not entirely sure why some of these test files are binary in the first place. Is it by mistake or intentionally... If those can be normal ASCII text files they definitely should be converted to that.

This should probably be done in a separate pull request/patch for easier handling. There are quite many binary phpt files - at least 292 from my quick check.

[carusogabriel]

I'm just not that fast with all the file fixing scripts

Don't worry, this patch is still in WIP and I've followed your recently patches, was just a ping.

This should probably be done in a separate pull request/patch for easier handling

Agree. let me know how we can fix it, so I can hold this patch to merge after this fix we want to.

[carusogabriel]

@petk Gonna remove the binary test files from this patch and open it for review. I'll post it here so we can track them in the future we want to solve this task. Sounds good?

[petk]

Sounds good...

[php-pulls]

Comment on behalf of carusogabriel at php.net:

Labelling

[carusogabriel]

Patch is ready for review.

[carusogabriel]

@petk Mind to review this one? :)

[petk]

It seems that CI passed this one. Is this change somehow repeatable? Using a script or a list of whitelisted/blacklisted files?

There is also one remaining binary files changed Zend/tests/cast_to_array_fixed.phpt.

[carusogabriel]

Is this change somehow repeatable? Using a script or a list of whitelisted/blacklisted files?

Nops. I trimmed with a lot of regex and add/remove files from git until get a perfect patch.

There is also one remaining binary files changed Zend/tests/cast_to_array_fixed.phpt.

I don't think is necessary to revert, as it passes in the CI and locally.

@cmb69 Mind to check on this one as well?

[cmb69]

Now, I'm not entirely sure why some of these test files are binary in the first place.

Some of our tests have ASCII control characters in the --EXPECT-- sections. In my opinion, this should be fixed in the long run, e.g. by using some kind of escape mechanism (e.g. bin2hex()) instead of printing such non-printable characters directly.

Wrt. each.phpt: such tremendous tests have the unfortunate consequence, that they'll be fixed by whatever is actually output, if they fail. In this case it seems that the test has been written this way in the first place, without even checking whether the tested output makes sense. The NUL bytes are the result of outputting this array, which has actually only 9 elements, even though 12 are written, and this has nothing to do with each().

Mind to check on this one as well?

I don't have the time to review more than 5000 changed files, but it seems to me we can commit this, if we check the submitted test reports for potential issues, subsequently.

[nikic]

Nops. I trimmed with a lot of regex and add/remove files from git until get a perfect patch.

Especially if reproducing this result is non-trivial, we need an automated way to do this before the change is committed.

[carusogabriel]

@nikic @petk Here's some of the patterns I used to compose this patch:

• Trim spaces after section, e.g. --TEST--
• Trim spaces after <?php, ?>, ) {, );, ;
• Trim spaces after phpDocs, e.g. /**
• Trim space after documentation, e.g. * Alias to functions:

Do you think is there a way to automate that, or can we proceed with the merge?

I don't have the time to review more than 5000 changed files, but it seems to me we can commit this, if we check the submitted test reports for potential issues, subsequently.

@cmb69 Thanks for the link. I used to check GCOV for that before, but I'll track QA as well.

[petk]

Thanks @cmb69 for the explanation. Yes, of course. Once you go through the binary files basics, it's more understandable what's happening. For the binary PHP test files (or any other binary files where diff is important in Git) a simple diff attribute can be added into the .gitattributes.

# .gitattributes
*.phpt diff

That way Git will show diffs for these files as well and still treat them as binary in the index and all as before.

[petk]

@carusogabriel I've added some of these fixing rules specific for phpt files to the upcoming tidy.php script in the #3505

Since this pull request is probably very nicely fine tuned I think we should merge it in the master anyway after we get most of the specifics for phpt files figured out. I'll try to get as close to this one in the tidy.php though. So, this is on its way and coming up soon.

[carusogabriel]

@petk Thanks for that.

If there's no objection, I'll be merging this in the weekend

[petk]

I think that's good. The tidy.php script is in final fine tuning phase... I think I got at least most of the phpt files figures out a bit. Not everything, but a lot:

• Clean completely the common known sections such as --TEST--, --DESCRIPTION--, --SKIPIF--, --INI--, --ENV--,--EXTENSIONS--, --CREDITS--, --FILE_EXTERNAL--.
• The --FILE-- and --FILEEOF-- cleaned by some more granular rules of PHP code (for example, trimming all whitespace is not ok, Like for example multiline echo statements and HEREDOC, but for the PHP code it is)
• All phpt files can have newlines also sorted out: (one final newline and redundant final newlines trimmed).
• For the CRLF vs LF I'm not there exactly yet, but will be also soon at the understanding where it's important, and where not..

[petk]

I only hope that 1000+ lines long tidy.php won't be an issue in php-src 😐

I'm currently doing dedicated tests of the tidy.php itself manually in not so fine tuned way yet...

[KalleZ]

@petk You shouldn't worry about the size, its the utility itself that matters ;-)

[nikic]

If there's no objection, I'll be merging this in the weekend

I'm still objecting -- we'll want to apply mass CS changes to all active branches, which is not possible in the current form. I would suggest to wait until @petk finishes his work and apply the phpt CS cleanup together with the other CS cleanup.

[carusogabriel]

@nikic So should I also prepare patches for the other active branches?

[petk]

Hello, the tidy.php script can now fix also *.phpt files. This one should be good to merge now...

[carusogabriel]

@petk Should I merge only on master, and then the script will take care about the other actives ones?

[petk]

yes, this should go only in master...

[carusogabriel]

Applied via 9c144e0.

I'll track both GCOV and QA to find possible regressions related to these changes.