unit tests should fail when warning are triggered

For the record, this is already the case. Any "php error" (notice, warning, error) is turned into an exception by PHPUnit and fails the test. Any "mediawiki error" (not debug/info) that goes to mw-exception.log or mw-error.log in Jenkins, will make the job fail per T50002. Our Jenkins jobs, after completing the PHPUnit run, assert that these files are empty/non-existent.

However, if they are turned into debug-level errors, then, naturally, those don't cause tests to fail given we have (and should have) plenty of debug messages being output normally.

@Krinkle wrote:

Any "php error" (notice, warning, error) is turned into an exception by PHPUnit and fails the test.

I know, I wrote that code :) My point is, calling LegacyLogger::log with LogLevel::WARNING does not do this. But should.

Any "mediawiki error" (not debug/info) that goes to mw-exception.log or mw-error.log in Jenkins, will make the job fail per T50002

That is correct for jenkins, but local tests will not fail. Also, for a vanilla install, such errors will be invisible on the live system, unless someone enables and monitors mw-exception.log.

Change 423035 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Make DeferredUpdates re-throw exceptions during testing.

https://gerrit.wikimedia.org/r/423035

@Krinkle wrote:

Any "php error" (notice, warning, error) is turned into an exception by PHPUnit and fails the test.

Just to clarify, I was referring to [convertErrorsToExceptions](https://phpunit.de/manual/4.8/en/appendixes.configuration.html).

We currently enable $wgDebugLogFile (which writes all severities and all channels of MediaWiki logging) in Jenkins' LocalSettings file for MediaWiki jobs. We also have separate files for specific channels (php-exception, php-error, and ratelimit).

Perhaps we can also configure it so that of all channels, messages with severity of WARNING and higher, are written emitted as php errors. That would naturally make them show up in stderrr and/or caught by PHPUnit.

I'm not entirely sure how yet, but I believe this would be possible using the modern $wgMWLoggerDefaultSpi configuration format (instead of the older wgDebugLogGroups config, which just feeds into that).

Change 423035 merged by jenkins-bot:
[mediawiki/core@master] Make DeferredUpdates re-throw exceptions during testing.

https://gerrit.wikimedia.org/r/423035

@Krinkle wrote:

Perhaps we can also configure it so that of all channels, messages with severity of WARNING and higher, are written emitted as php errors. That would naturally make them show up in stderrr and/or caught by PHPUnit.

I think that would be a good idea. I looked into doing that, but got very confused by LegacyLogger.

For the record: a stop gap solution is merged, but it's a Volkswagen solution: DeferredUpdates changes behavior based on whether it's invoked during a unit test.

In any case, critical errors thrown in DeferredUpdates should not be invisible per default on 3rd party installations. This ticket should stay open until that problem is resolved.

In any case, critical errors thrown in DeferredUpdates should not be invisible per default on 3rd party installations. This ticket should stay open until that problem is resolved.

Just want to add a giant +1 to that (after spending a couple hours trying to track down errors which i assumed didn't exist because they didn't show up in my php error log despite E_DEPRECATED things from the same code showing up there).

Moving on workboard from Backlog to Next, since the Backlog column was hidden some time ago. Is there work remaining to do on this task?

@daniel Is there work remaining to do on this task?

Is this resolved?

In T190178#6739785, @holger.knust wrote:

Is this resolved?

I don't know... Krinkle said above that the original issue has since been fixed, but I don't know anything about that.

To test, make a DeferredUpdate that throws an exception and schedule it with DeferredUpdates::addUpdate() in an integration test. It should make the test fail. Ideally, we'd have a test that tests this :)

--- a/tests/phpunit/integration/includes/user/UserFactoryTest.php
+++ b/tests/phpunit/integration/includes/user/UserFactoryTest.php
@@ -24,6 +24,11 @@ class UserFactoryTest extends MediaWikiIntegrationTestCase {
        $user = $factory->newFromName( $name );
 
        $this->assertInstanceOf( User::class, $user );
+
+       DeferredUpdates::addCallableUpdate( function () {
+           throw new Exception( 'Boo!' );
+       } );
+
        $this->assertSame( $name, $user->getName() );
    }

# php phpunit.php integration/includes/user/UserFactoryTest.php 
Using PHP 7.2.32
PHPUnit 8.5.9 by Sebastian Bergmann and contributors.

E.........                                                        10 / 10 (100%)

Time: 6.36 seconds, Memory: 40.25 MB

There was 1 error:

1) UserFactoryTest::testNewFromName
Exception: Boo!

/var/www/mediawiki/tests/phpunit/integration/includes/user/UserFactoryTest.php:29

In addition, in case it somehow escapes PHPUnit, it also goes to mw-error.log, which CI fails jobs for if non-empty:

[DeferredUpdates] [debug] DeferredUpdates::run: started MWCallableUpdate_UserFactoryTest->testNewFromName #889 []
[DeferredUpdates] [debug] DeferredUpdates::run: ended MWCallableUpdate_UserFactoryTest->testNewFromName #889 []

[exception] [error] [{exception_id}] {exception_url}   Exception from line 29 of /var/www/mediawiki/tests/phpunit/integration/includes/user/UserFactoryTest.php: Boo! {"exception":… "Call Stack: … 7. UserFactoryTest->run() \/var\/www\/mediawiki\/vendor\/phpunit\/phpunit\/src\/Framework\/TestSuite.php:601\n    5.5336   36983984   8. UserFactoryTest->run() \/var\/www\/mediawiki\/tests\/phpunit\/MediaWikiIntegrationTestCase.php:446\n    5.5337   36983984   9. PHPUnit\\Framework\\TestResult->run() \/var\/www\/mediawiki\/vendor\/phpunit\/phpunit\/src\/Framework\/TestCase.php:763\n    5.5443   37074752  10. UserFactoryTest->runBare() \/var\/www\/mediawiki\/vendor\/phpunit\/phpunit\/src\/Framework\/TestResult.php:691\n    5.6658   37699328  11. UserFactoryTest->onNotSuccessfulTest() \/var\/www\/mediawiki\/vendor\/phpunit\/phpunit\/src\/Framework\/TestCase.php:1133\n"},"exception_id":"f4e6cc7f3a7686349515a499","exception_url":"[no req]","caught_by":"other"}