phpunit on PHP 7.2 or 7.3 can exit with 0 status when there are test fails

[phil-davis]

See PR #34972 and #34857 for a demo of the behavior:

If I break tests/lib/legacy/AppTest.php (see first commit for dumb easy example), then the phpunit test runs on drone:

• fail when running with phpdbg and collecting coverage (all the drone coverage build jobs correctly report the 6 purposely induced unit test fails and exit with non-zero status) e.g. https://drone.owncloud.com/owncloud/core/16325/29 and https://drone.owncloud.com/owncloud/core/16325/38
• fail when running "ordinary" php7.1 and phpunit (also correctly reports the 6 purposely induced unit test fails... like the previous point) e.g. https://drone.owncloud.com/owncloud/core/16325/47
• pass when running "ordinary" php7.2 or 7.3 and phpunit (the 6 purposely induced unit test fails are logged by phpunit but phpunit exits with status 0) this is a bad thing

PR #34972 has a cut-down example of the offending behavior.

• the first commit purposely puts a fail into tests/lib/legacy/AppTest.php so that we expect to get 6 test fails.
• the 2nd commit:
  • cuts down the drone jobs so just a few sqlite phpunit jobs are run
  • cuts down tests/phpunit-autotest.xml to not bother running unit tests that did not effect the result
  • cuts down tests/apps.php so it only runs tests from the files app
  • cuts down the tests for the files app to just apps/files/tests/Command/ScanTest.php
  • cuts down the content of ScanTest to just a single simplified test case

Running this results in the above behavior - some PHP7.2 and PHP 7.3 unit test runs come out with a "pass" on drone when actually 6 tests failed.

PR #34857 also has:

• the 3rd commit moves ScanTest.php out of the way, and puts the other Command tests back. In drone job https://drone.owncloud.com/owncloud/core/16326 all the phpunit jobs correctly report failure of 6 unit tests.

That demonstrates that it is something to do with ScanTest that messes up phpunit on PHP 7.2 and 7.3

[phil-davis]

@PVince81 I noticed this behavior when looking at unit test results related to the AppTest.php problems you were having yesterday.

This is a separate issue, but it is concerning that "we" can cause phpunit to exit with "success" when it actually has reported test fails.

[PVince81]

any update ? was the attempted PHPUnit update related to solving this ?

[phil-davis]

I tried on PHPUnit6 and PHPUnit7 - the example way to induce failing tests (in original post at top) fails the test(s) but still exits with status 0, so bumping major versions of PHPUnit does not help.

I started trying with PHPUnit8, but that has more backward-compatibility breaks, which requires more test code refactoring to make it run. And it only supports PHP7.2+ so it would be nowhere near being useable by us just yet anyway. So I will not spend time on that!

Now that we dropped PHP5.6, the PHPUnit6 bump is good to have anyway. PHPUnit5 is a long way out of support. So I listed that as a task for "drop PHP5.6". The core PRs are ready.

I have a passing PHPUnit7 PR in core master. That will be useful when we drop PHP7.0 - it can be easily applied to master and backported to stable10. So I will leave it there and mark it "blocked". We do not want to merge it just to master - that will create confusion between master and stable10 for no good reason.

[phil-davis]

The next step for this issue could be:

1. Research more the offending test case that seems to break the PHPUnit exit status. Then:
   a) fix it up so it does not "break" PHPUnit; and/or
   b) make an issue in phpunit with a nice cut-down example, so that phpunit can be made more robust
2. Wrap script logic around our calls of phpunit so that when the exit status is 0 it parses the output, looking for "fail", "error",... tests reported and if so exits with an error status.

[patrickjahns]

@phil-davis
Please check owncloud/QA#550

In order to be 100% certain that a test succeeded, we should pass the results from phpunits log/output and base the assessment on this

[phil-davis]

^ @patrickjahns yes, I have come to that conclusion - phpunit 6 and 7 have the same exit status misbehavior, so it has not been corrected in phpunit in the last few years.

[phil-davis]

Maybe do this after we drop PHP7.0 and move up to phpunit7.
That will avoid having to even think about any differences that there might be in phpunit output between major version 6 and 7.

[phil-davis]

I haven't seen this problem for a long time. We are on phpunit 9.5 now.
Closing here. If this gets noticed again, then raise a new issue.