Page MenuHomePhabricator
Create Task
Maniphest T301100

Fix flaky ContributionsCount.js test
Open, HighPublic
Actions

Description

As seen in https://integration.wikimedia.org/ci/job/integration-quibble-fullrun-extensions/4/console:

17:46:36   1) GET /contributions/count
17:46:36        GET /me/contributions/count
17:46:36          Returns edits filtered by tag:
17:46:36 
17:46:36       AssertionError: expected 0 to deeply equal 1
17:46:36       + expected - actual
17:46:36 
17:46:36       -0
17:46:36       +1
17:46:36       
17:46:36       at testGetContributionsCountByTag (tests/api-testing/REST/ContributionsCount.js:60:10)
17:46:36       at processTicksAndRejections (internal/process/task_queues.js:95:5)
17:46:36       at async Context.<anonymous> (tests/api-testing/REST/ContributionsCount.js:117:4)
17:46:36 
17:46:36   2) GET /contributions/count
17:46:36        GET /user/{user}/contributions/count
17:46:36          Returns edits filtered by tag:
17:46:36 
17:46:36       AssertionError: expected 0 to deeply equal 1
17:46:36       + expected - actual
17:46:36 
17:46:36       -0
17:46:36       +1
17:46:36       
17:46:36       at testGetContributionsCountByTag (tests/api-testing/REST/ContributionsCount.js:60:10)
17:46:36       at processTicksAndRejections (internal/process/task_queues.js:95:5)
17:46:36       at async Context.<anonymous> (tests/api-testing/REST/ContributionsCount.js:151:4)
17:46:36

It seems this is a case of the API request for contributions count being handled before the data is committed in MySQL. There are already existing uses of sleep() in the api-testing codebase that work around similar problems but it would be better to have a try/retry loop for n seconds, like wdio's waitUntil() method for browser tests.

In the meantime, I'll submit a patch to disable this test.

Details

SubjectRepoBranchLines +/-
DeferredUpdates: Introduce $wgForceDeferredUpdatesPreSendmediawiki/coremaster+70 -13
api-testing: Disable failing contributions count testsmediawiki/coremaster+8 -4
api-testing: Skip flaky contributions count testmediawiki/coremaster+4 -2
api-testing: Skip flaky testmediawiki/coremaster+4 -2
Customize query in gerrit

Related Objects
Search...

Event Timeline

kostajh created this task.Feb 7 2022, 8:36 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 7 2022, 8:36 AM
gerritbot added a comment.Feb 7 2022, 8:38 AM

Change 760519 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] api-testing: Skip flaky test

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

gerritbot added a project: Patch-For-Review.Feb 7 2022, 8:38 AM
kostajh added a parent task: T285649: Switch all Quibble Selenium and api-testing jobs to use apache.Feb 7 2022, 8:38 AM
kostajh added a parent task: T299492: Quibble ci-fullrun jobs should use Apache backend.Feb 7 2022, 10:55 AM
gerritbot added a comment.Feb 8 2022, 1:35 PM

Change 760519 merged by jenkins-bot:

[mediawiki/core@master] api-testing: Skip flaky test

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.22; 2022-02-14).Feb 8 2022, 2:00 PM
Maintenance_bot removed a project: Patch-For-Review.Feb 8 2022, 2:10 PM
gerritbot added a comment.Feb 8 2022, 2:29 PM

Change 760952 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] api-testing: Skip flaky contributions count test

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

gerritbot added a project: Patch-For-Review.Feb 8 2022, 2:29 PM
daniel added projects: User-Daniel, Testing-Roadblocks.Feb 8 2022, 5:40 PM
gerritbot added a comment.Feb 8 2022, 5:58 PM

Change 760952 merged by jenkins-bot:

[mediawiki/core@master] api-testing: Skip flaky contributions count test

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

Maintenance_bot removed a project: Patch-For-Review.Feb 8 2022, 6:10 PM
daniel subscribed.Feb 9 2022, 12:13 PM

Larger context: T230211: Ensure that GET requests in API integration tests will always see the effect of previous POST requests.

daniel mentioned this in T230211: Ensure that GET requests in API integration tests will always see the effect of previous POST requests..Feb 9 2022, 12:25 PM

From a comment by Timo on T230211:

If it does need to work with php-fpm, then I recommend setting postSendStrategy = DEFER_SET_LENGTH_AND_FLUSH instead of postSendStrategy = DEFER_FASTCGI_FINISH_REQUEST, which will give you the same treatment. We could make this configurable and perhaps even the default for CI and DevelopmentSettings.php.

daniel triaged this task as High priority.Feb 9 2022, 12:26 PM
kostajh added a comment.Feb 9 2022, 12:42 PM

I'm sorry, it's possible that these tests aren't the problem, because now I see that the Quibble extensions fullrun build is failing in the PHPUnit integration test phase. Let me see if I can figure out what is wrong with the Quibble build, e.g. https://integration.wikimedia.org/ci/job/integration-quibble-fullrun-extensions/10/console#console-section-13

gerritbot added a comment.Feb 9 2022, 1:13 PM

Change 761320 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] DeferredUpdates: configure update strategy.

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

gerritbot added a project: Patch-For-Review.Feb 9 2022, 1:13 PM
gerritbot added a comment.Feb 15 2022, 7:16 PM

Change 761320 merged by jenkins-bot:

[mediawiki/core@master] DeferredUpdates: Introduce $wgForceDeferredUpdatesPreSend

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.23; 2022-02-21); removed MW-1.38-notes (1.38.0-wmf.22; 2022-02-14).Feb 15 2022, 8:00 PM
Maintenance_bot removed a project: Patch-For-Review.Feb 15 2022, 8:10 PM
gerritbot added a comment.Feb 16 2022, 3:29 PM

Change 763262 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/core@master] api-testing: Disable failing contributions count tests

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

gerritbot added a project: Patch-For-Review.Feb 16 2022, 3:29 PM
daniel added a comment.EditedFeb 16 2022, 3:58 PM

So my patch for forcing deferred updates to happen immediately didn't fix the issue with ContributionsCount.js.

Looking at the code some more, this seems plausible: user contributions are computed from the revision table(*), which is always updated immediately by an edit. Updates to links tables and such are deferred, and updating the search index may be done via the job queue, but the revision table should always have the latest data immediately.

This leaves replication lag as a possible cause - but we are not using a DB replica on the text instance.

So we must be hitting a cache somewhere. But where?

(*) use_editcount exists and it's updated deferred, by UserEditCountUpdate. But ContributionsCountHandler uses ContributionsLookup::getContributionCount() which does a COUNT(*) on the revision table.

gerritbot added a comment.Feb 16 2022, 4:22 PM

Change 763262 merged by jenkins-bot:

[mediawiki/core@master] api-testing: Disable failing contributions count tests

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

kostajh added a comment.Feb 16 2022, 9:07 PM
In T301100#7715058, @daniel wrote:

So my patch for forcing deferred updates to happen immediately didn't fix the issue with ContributionsCount.js.

Looking at the code some more, this seems plausible: user contributions are computed from the revision table(*), which is always updated immediately by an edit. Updates to links tables and such are deferred, and updating the search index may be done via the job queue, but the revision table should always have the latest data immediately.

This leaves replication lag as a possible cause - but we are not using a DB replica on the text instance.

So we must be hitting a cache somewhere. But where?

(*) use_editcount exists and it's updated deferred, by UserEditCountUpdate. But ContributionsCountHandler uses ContributionsLookup::getContributionCount() which does a COUNT(*) on the revision table.

Is ChronologyProtector somehow involved?

daniel added a comment.EditedFeb 17 2022, 9:07 AM
In T301100#7716339, @kostajh wrote:

Is ChronologyProtector somehow involved?

I don't see how. ChronologyProtector prevents a user seeing inconsistent state because of replication lag. But we don't have DB replication on the CI system. Besides, ChronologyProtector would be insufficient to prevent inconsistencies between multiple users, which would be needed for consistent testing.

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