Page MenuHomePhabricator

Jenkins build for MediaWiki should fail when "PHP Warning" is emitted
Open, MediumPublicPRODUCTION ERROR

Description

At https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/481954/, the build at https://integration.wikimedia.org/ci/job/mediawiki-quibble-vendor-mysql-php70-docker/12101/consoleFull contained the following output:

php tests/phpunit/phpunit.php --debug-tests --exclude-group Broken,ParserFuzz,Stub,Database --log-junit /workspace/log/junit-dbless.xml

Using PHP 7.0.30-0+deb9u1

PHP Warning:  Declaration of MediaWikiPageNameNormalizerTestMockHttp::get($url, $options = Array, $caller = 'MediaWikiP...') should be compatible with Http::get($url, array $options = Array, $caller = 'Http::get') in /workspace/src/tests/phpunit/includes/site/MediaWikiPageNameNormalizerTest.php on line 103

PHPUnit 6.5.13 by Sebastian Bergmann and contributors. 
...........................................................    59 / 12990 (  0%)
...........................................................   236 / 12990 (  1%)

There are two reasons this build should've failed:

  • Our PHPUnit configuration turns warnings into exceptions, which naturally make the phpunit process exit with errors, and thus fail the build.
  • MediaWiki channels any PHP warnings into mw-error.log which our Jenkins job asserts it empty/non-existent, and fails the build if not. / T50002

Event Timeline

The warning comes before the "PHPUnit 6.5.13" line, which I think means it was emitted before our PHPUnit configuration was loaded, so the warnings-into-errors doesn't take effect yet, nor the MediaWiki config.

@Legoktm Aha, right. I forgot that we don't actually capture the PHP stderr.

Back in 2013 with T50002, it was difficult to capture all relevant PHP error output because the Jenkins nodes had a shared Apache and PHP-CGI for multiple Jenkins jobs running a build on that node. Thus, we opted to manage it in-process. By capturing php-error events from within PHPUnit and MediaWiki. The downside of course is that we miss everything that isn't triggered within a specific test. Because all initialisation and autoloading happens before we start watching for errors.

Today in 2019, this is no longer the case. We use Quibble and have a dedicated PHP server. We can configure it via php.ini to make the error.log file be in the log/ directory. Then, the Jenkins logic we have that checks for mw-{error,exception}.log, can check this file as well.

Krinkle triaged this task as Medium priority.Apr 5 2019, 8:55 PM
Krinkle added a project: Quibble.
mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:07 PM
daniel subscribed.

Tagging CPT. We might want to look into this. Or at least have it on our radar.

Here we go again in 2024, when you can submit a change that redeclares an existing class with PHP just being like "oh, you're declaring the same class twice, but surely this isn't serious enough for me to stop you, please go ahead" and CI not batting an eye if you merge it. I think the CI solution is fine, but even better would be catching all warnings from the autoloader and turning them into fatals. As I noted in T378774#10283126, doing it unconditionally may be bad for performance ({{cn}}, though); but maybe we could do it for tests only.