Page MenuHomePhabricator

ChangesListSpecialPageTest::testFilterUserExpLevel fails when coverage is enabled
Closed, ResolvedPublic

Description

https://integration.wikimedia.org/ci/job/mediawiki-core-code-coverage/2648/console
[..]
https://integration.wikimedia.org/ci/job/mediawiki-core-code-coverage/2750/console

1) ChangesListSpecialPageTest::testFilterUserExpLevel
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'Newcomer1'
-    1 => 'Newcomer2'
-    2 => 'Newcomer3'
+    0 => 'Experienced1'
+    1 => 'Learner1'
+    2 => 'Learner2'
+    3 => 'Learner3'
+    4 => 'Learner4'
+    5 => 'Newcomer1'
+    6 => 'Newcomer2'
+    7 => 'Newcomer3'
 )

/srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/src/tests/phpunit/MediaWikiTestCase.php:1512
/srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/src/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php:418
/srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/src/tests/phpunit/MediaWikiTestCase.php:400
/srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/src/maintenance/doMaintenance.php:111

Jenkins runs PHPUnit as follows:

+ nice -n 19 php /srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/src/tests/phpunit/phpunit.php --exclude-group Dump,Broken,ParserFuzz,Stub --coverage-clover log/clover.xml --coverage-html /srv/jenkins-workspace/workspace/mediawiki-core-code-coverage/cover

I imagine the following should suffice to reproduce it:

mediawiki/tests/phpunit$ php phpunit.php --coverage-html ../coverage

See also https://www.mediawiki.org/wiki/Manual:PHP_unit_testing/Code_coverage for info specific to MediaWiki-Vagrant.

Event Timeline

Krinkle created this task.Mar 21 2017, 9:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 21 2017, 9:09 PM
Krinkle updated the task description. (Show Details)Mar 21 2017, 9:10 PM
Krinkle added subscribers: Catrope, Mattflaschen-WMF.
Krinkle updated the task description. (Show Details)Mar 21 2017, 9:19 PM
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)Apr 18 2017, 3:47 AM

ChangesListSpecialPageTest::testFilterUserExpLevel is passing for me locally, with or without coverage enabled.

It is possible that this is running against a different DB, one where date comparisons behave differently?

ChangesListSpecialPageTest::testFilterUserExpLevel is passing for me locally, with or without coverage enabled.
It is possible that this is running against a different DB, one where date comparisons behave differently?

I assume you're selectively running only one test suite. This inherently runs much faster and is unlikely to trigger the necessary race conditions for this problem. This test usually passes consistently, even on Jenkins. It only fails when generating the code coverage.

So far all coverage-only failures I've seen are due to a hardcoded assumptions about time, which don't hold up when execution is slowed down by a factor for 5-10x. In addition, it is important to know that all test fixtures and data providers are collected before the first test starts. That is the most significant delay by far.

This is difficult to reproduce locally and would likely take 20min or so to run (no extensions, just core, but otherwise default settings/unfiltered). But in the other cases I was able to find the culprit without reproducing it first by critically examining the involved code for any references to timestamps.

I've tried running only this one or running them all with coverage. This test doesn't have a provider and doesn't rely on anything from a setUp() function. It starts by creating its users then performs a bunch of tests on them. Adding a 10-minute delay after the user creation doesn't change anything locally.

It definitely contains assumptions about time (it creates users with fake registration dates based on time() - X days) but for all the users to be returned when we ask only for beginners, it would have to be delayed by over 30 days and they would need to have made 500+ edits in the meantime (because those fake users where bored while the tests were being prepared ;) ).

Here's a crazy hypothesis: the code coverage instrumentation messes with the pass-by-ref parameters so that the $conds variable used to build the db query in the test (ChangesListSpecialPageTest::fetchUsers) doesn't contain the where clause added by the method under test (ChangesListSpecialPage::filterOnUserExperienceLevel). It don't see this happening locally but we can test it in a patch if you can run the full coverage in Jenkins on any branch.

Change 349456 had a related patch set uploaded (by Sbisson):
[mediawiki/core@master] userExpLevel test: use $tables from function under tests

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

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

This patch uses the $tables variable to fetch the users in the tests. In case of a pass-by-ref issue, the test would fail with a malformed SQL exception because no table would be specified.

SBisson claimed this task.Apr 21 2017, 5:31 PM

Claiming since I've started investigating but anybody with a potential solution is welcome to chime in.

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

This patch uses the $tables variable to fetch the users in the tests. In case of a pass-by-ref issue, the test would fail with a malformed SQL exception because no table would be specified.

With @Krinkle 's help I triggered a coverage test run on your patch, so let's see what comes out of that.

Instructions for posterity (require logging into Jenkins as an admin):

13:07:57 <+Krinkle> RoanKattouw: Go to the commit you wanna test against, pick a random job that ran for it that relates to phpunit, click rebuild, but don't submit. Then go to the mediawiki code coverage job and rebuild last, then take the 'relevant' vars from one and enter into the other and trigger the build. Most important is ZUUL_REF project and branch

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

This patch uses the $tables variable to fetch the users in the tests. In case of a pass-by-ref issue, the test would fail with a malformed SQL exception because no table would be specified.

Took another look through the code, I'd recommend also passing $time to daysAgo() and set it only once, in testFilterUserExpLevel, so that it is the same for all users.

Change 349951 had a related patch set uploaded (by Sbisson):
[mediawiki/core@master] userExpLevel test: use a single time()

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

Took another look through the code, I'd recommend also passing $time to daysAgo() and set it only once, in testFilterUserExpLevel, so that it is the same for all users.

The following patch does that and propagate the $now value to the production code.

Change 349951 had a related patch set uploaded (by Sbisson):
[mediawiki/core@master] userExpLevel test: use a single time()
https://gerrit.wikimedia.org/r/349951

Change 349951 had a related patch set uploaded (by Sbisson):
[mediawiki/core@master] userExpLevel test: use a single time()
https://gerrit.wikimedia.org/r/349951

I've kicked off a coverage test run for this patch: https://integration.wikimedia.org/ci/job/mediawiki-core-code-coverage-jessie/17/console

...and it still failed.

Change 350894 had a related patch set uploaded (by Sbisson; owner: Sbisson):
[mediawiki/core@master] userExpLevelTest: output relevant data

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

User Newcomer1, registration: 2017-04-26 21:06:01
User Newcomer2, registration: 2017-04-25 21:06:02
User Newcomer3, registration: 2017-04-23 21:06:02
User Learner1, registration: 2017-04-18 21:06:02
User Learner2, registration: 2017-04-08 21:06:02
User Learner3, registration: 2017-03-26 21:06:02
User Learner4, registration: 2017-03-31 21:06:03
User Experienced1, registration: 2017-03-26 21:06:03

fetchUsers conds: Array
(
    [0] => NOT ( (user_editcount >= 10) AND (user_registration <= 2017-04-24 21:06:03) )
)

Change 349951 merged by jenkins-bot:
[mediawiki/core@master] userExpLevel test: use a single time()

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

Change 349456 merged by jenkins-bot:
[mediawiki/core@master] userExpLevel test: use $tables from function under tests

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

Krinkle updated the task description. (Show Details)May 6 2017, 12:21 AM
SBisson closed this task as Resolved.May 10 2017, 11:35 AM

Change 350894 abandoned by Sbisson:
userExpLevelTest: output relevant data

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

hashar added a subscriber: hashar.Apr 19 2018, 2:59 PM

I encountered that failure when using SQLite as a database backend in MW-1.29-release

Change 427677 had a related patch set uploaded (by Hashar; owner: Aaron Schulz):
[mediawiki/core@REL1_29] Add missing addQuotes() to ChangesListSpecialPage

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

Change 427677 merged by jenkins-bot:
[mediawiki/core@REL1_29] Add missing addQuotes() to ChangesListSpecialPage

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