Page MenuHomePhabricator
Create Task
Maniphest T161041

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

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.

Details

SubjectRepoBranchLines +/-
Add missing addQuotes() to ChangesListSpecialPagemediawiki/coreREL1_29+5 -2
userExpLevelTest: output relevant datamediawiki/coremaster+11 -2
userExpLevel test: use $tables from function under testsmediawiki/coremaster+1 -1
userExpLevel test: use a single time()mediawiki/coremaster+20 -16
Customize query in gerrit

Related Objects

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)
• Mattflaschen-WMF added a project: Collaboration-Team-Triage (Collab-Team-Q3-Jan-Mar-2017).Mar 21 2017, 11:57 PM
Krinkle mentioned this in T160956: WatchedItemStoreIntegrationTest fails when coverage is enabled.Mar 22 2017, 9:23 PM
Catrope moved this task from Untriaged to Ready for Pickup on the Collaboration-Team-Triage (Collab-Team-Q3-Jan-Mar-2017) board.Mar 28 2017, 5:53 PM
• jmatazzoni edited projects, added Collaboration-Team-Triage (Collab-Team-Q4-Apr-Jun-2017); removed Collaboration-Team-Triage (Collab-Team-Q3-Jan-Mar-2017).Apr 18 2017, 12:58 AM
• jmatazzoni moved this task from Untriaged to Ready for Pickup on the Collaboration-Team-Triage (Collab-Team-Q4-Apr-Jun-2017) board.Apr 18 2017, 1:01 AM
Krinkle updated the task description. (Show Details)Apr 18 2017, 3:47 AM
SBisson subscribed.Apr 18 2017, 1:31 PM

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?

Krinkle added a comment.Apr 19 2017, 12:30 AM
In T161041#3189683, @SBisson wrote:

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.

SBisson added a comment.Apr 21 2017, 4:11 PM

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.

gerritbot subscribed.Apr 21 2017, 4:17 PM

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

gerritbot added a project: Patch-For-Review.Apr 21 2017, 4:17 PM
SBisson added a comment.Apr 21 2017, 4:19 PM

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
SBisson moved this task from Ready for Pickup to In Development on the Collaboration-Team-Triage (Collab-Team-Q4-Apr-Jun-2017) board.

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

Catrope added a comment.Apr 21 2017, 8:28 PM
In T161041#3201807, @SBisson wrote:

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
Krinkle added a comment.Apr 22 2017, 4:45 AM
In T161041#3201807, @SBisson wrote:

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.

gerritbot added a comment.Apr 24 2017, 12:56 PM

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

SBisson added a comment.Apr 24 2017, 12:59 PM
In T161041#3203227, @Krinkle wrote:

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.

In T161041#3205804, @gerritbot wrote:

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

Catrope added a comment.Apr 25 2017, 2:00 AM
In T161041#3205804, @gerritbot wrote:

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

SBisson added a comment.Apr 25 2017, 11:11 AM
In T161041#3208596, @Catrope wrote:

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

Thanks. It failed the same way :(

Catrope added a comment.Apr 25 2017, 7:34 PM

...and it still failed.

gerritbot added a comment.Apr 28 2017, 7:37 PM

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

SBisson added a comment.May 1 2017, 12:47 PM
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) )
)
gerritbot added a comment.May 2 2017, 6:43 PM

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

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 2 2017, 7:00 PM
gerritbot added a comment.May 2 2017, 7:58 PM

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

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

ReleaseTaggerBot edited projects, added MW-1.30-release-notes; removed MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 2 2017, 8:00 PM
Krinkle updated the task description. (Show Details)May 6 2017, 12:21 AM
Krinkle removed projects: MW-1.30-release-notes, Patch-For-Review.May 9 2017, 7:56 PM
Krinkle updated the task description. (Show Details)
SBisson added a comment.May 10 2017, 11:34 AM

It looks like this was fixed by rMWdbfbbf9cb887: Add missing addQuotes() to ChangesListSpecialPage

SBisson closed this task as Resolved.May 10 2017, 11:35 AM
gerritbot added a comment.May 12 2017, 11:19 AM

Change 350894 abandoned by Sbisson:
userExpLevelTest: output relevant data

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

Krinkle awarded a token.May 25 2017, 2:31 PM
hashar subscribed.Apr 19 2018, 2:59 PM

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

gerritbot added a comment.Apr 19 2018, 3:01 PM

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

gerritbot added a project: Patch-For-Review.Apr 19 2018, 3:01 PM
hashar mentioned this in T192372: [tracking] Get quibble to run on all branches/php/database.Apr 19 2018, 3:01 PM
gerritbot added a comment.Apr 19 2018, 10:05 PM

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

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

ReleaseTaggerBot added a project: MW-1.29-release-notes.Apr 19 2018, 11:00 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL