Remove ignored lines from tests

[villfa]

In the FILE section every lines after ===DONE=== are ignored.
These lines give the false impression that they have a reason to be there.
Removing these lines removes also this ambiguity.

[nikic]

See https://qa.php.net/write-test.php#lastbit for why these exist. Personally I don't think I ever cared about this and I don't think anyone does it for new tests, so I think it's okay to drop them if we like.

However, if we do we should also drop the ===DONE=== as well. We should only keep ===DONE=== if the test output otherwise becomes empty (which I think is the only context in which it is used nowadays).

[villfa]

@nikic The unnecessary ===DONE=== tags are removed like you suggested. There are 196 left (on the PHP-7.4 branch).

Do you agree that I also remove the part about this tag from run-test.php?
Or maybe it can be in another PR targeting master only?

[krakjoe]

I see thousands of files changed, I don't think it appropriate to merge this into 7.4 ... generally we merge test changes like this into master only ...

I'm neutral on the actual change, but if it's going to happen please make pulls for qa and anywhere else this is documented.

[villfa]

@krakjoe the PR now targets master

After thinking it through I'm reluctant to remove this feature entirely since it causes no harm and I don't know how it is used by third-party PHP extensions.

I still think this PR has value though:
at some point writing tests with the ===DONE=== tag has become a convention and just a few people knows it has a side effect. (I base my remark on the thousand of tests with the tag wrongly used)
Cleaning the tests this way should stop encouraging this wrong usage.

[nikic]

I haven't reviewed all the changes, assuming everything is as expected here...

Agree that support should not be dropped from run-tests.php, as 3rd party tests are definitely using this, and having an exit(0) in a test causes a lot of damage, we definitely do not want that to accidentally run.

[bukka]

@nikic @villfa Couldn't this result in missing some crashes. For example if a555cc0#diff-3ae61fee455644f3a3c763c005fdbf57L12 openssl_csr_new crashes, how it could be found?

[villfa]

@bukka The presence or the absence of ===DONE=== should not affect the tests.
Do you have a specific test in mind? I couldn't tell from the link above.

[nikic]

@bukka A message is appended to the test output if it exits with an unusual exit code.

[bukka]

@nikic Would that still be the case if the library / extension exited successfully (e.g. calling C exit(0)) somehow because of the bug?

[guilliamxavier]

I share @bukka's concerns... Take e.g.:

--TEST--
Dummy example
--FILE--
<?php
// the following call is expected to "println" its arg then let the script continue:
some_function('Hello world!');
?>
Done.
--EXPECT--
Hello world!
Done.

If a bug in the implementation of some_function makes it exit with code 0 after printing, the bug will be detected by the test because the expected Done. line will be missing in the actual output.
But now if you remove the Done. lines (in both --FILE-- and --EXPECT-- sections) then the bug won't be detected anymore...

In general I like the last line of a test to be a hard output, to make sure everything is executed to the end.