Page MenuHomePhabricator

wmf-quibble-vendor-mysql-php72-docker failed on Special:EntityData
Closed, ResolvedPublic

Description

Seen on ConfirmEdit (CAPTCHA extension) patch https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ConfirmEdit/+/588051/

https://integration.wikimedia.org/ci/job/wmf-quibble-vendor-mysql-php72-docker/29773/consoleFull

18:23:41 15 04 2020 17:23:41.181:DEBUG [middleware:source-files]: Requesting /index.php/Special:EntityData/null.json?revision=0
18:23:41 15 04 2020 17:23:41.182:DEBUG [middleware:source-files]: Fetching /index.php/Special:EntityData/null.json
18:23:41 15 04 2020 17:23:41.182:DEBUG [proxy]: proxying request - /index.php/Special:EntityData/null.json?revision=0 to 127.0.0.1:9412
18:23:41 15 04 2020 17:23:41.669:DEBUG [HeadlessChrome 73.0.3683 (Linux 0.0.0)]: CONFIGURING -> EXECUTING
18:23:41 WARN: 'Pending requests does not match jQuery.active count'
18:23:41   testrunner
18:23:41     ✖️ Setup
18:23:41 WARN: 'Pending requests does not match jQuery.active count'
18:23:41     ✖️ Teardown
18:23:41 WARN: 'Pending requests does not match jQuery.active count'
18:23:41     ✖️ Loader status
18:23:41 WARN: 'Pending requests does not match jQuery.active count'
18:23:41     ✖️ assert.htmlEqual
18:23:41 WARN: 'Pending requests does not match jQuery.active count'
18:23:41   testrunner-after
18:23:41     ✖️ Teardown

Event Timeline

Reedy created this task.Apr 15 2020, 5:35 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptApr 15 2020, 5:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Reedy renamed this task from wmf-quibble-vendor-mysql-php72-docker failed to wmf-quibble-vendor-mysql-php72-docker failed on Special:EntityData.Apr 15 2020, 5:41 PM

Brings about memories of T189228 & T189329

Interestingly, the suspicious requests against Special:EntityData/null.json?revision=0 are also happening on successful builds (e. g. the one subsequent to the randomly failing one that led to this ticket). It should be fixed but is possibly not at fault.

Looking at the combination of the null entity id and the specified revision I'm tempted to assume the request stems from where we try and fire the wikibase.entityPage.entityLoaded hook.

Suspicious requests against Special:EntityData/null.json?revision=0 happen in qunit tests when wikibase.entityPage.entityLoaded.js gets loaded and instantaneously run (even on no particular test, e. g.). RL Module Terminators Trailblazing may have had an effect on this. Guarding it against null entity ids could be a (naive) way forward.

Preventing this outgoing AJAX request may, like in the past, ensure that there are no pending AJAX requests upon afterEach. Chances are this only happens rarely because most of the time the response is quick enough.

That sounds reasonable to me. If I understand correctly, the wikibase.entityPage.entityLoaded module gets loaded because it’s a dependency of wikibase.ui.entityViewInit, which in turn is required by wikibase.view.tests (see view/tests/qunit/resources.php). Loading entityViewInit for the tests seems reasonable to me, and I don’t think we can/should make the entityLoaded module a conditional dependency, so the entityLoaded module probably has to deal with the possibility of being loaded in the unit tests, when there is no entity ID.

I wonder if it would make sense to check whether we’re in a unit test in entityLoaded? Something like this:

  • If the entity ID is null:
    • If we’re in a unit test:
      • That’s fine, no need to worry about it. No-op.
    • If we’re not in a unit test:
      • There’s a bug, emit at least an mw.log.warn().
  • If the entity ID is not null:
    • Make the request. (Don’t bother checking if we’re in a unit test.)

I’m not sure what the best way to test “are we in a test” is – I found a check for window.QUnit being defined in a MediaWiki core file (resources/src/startup/mediawiki.js), but that’s the startup module, which is special in many ways. Maybe that kind of check is discouraged in other code?

Pablo-WMDE added a comment.EditedApr 16 2020, 1:47 PM

If the entity ID is null: ...

I conceptually agree with the tree of options you drew but don't think we can responsibly implement the branch "If the entity ID is null > If we’re in a unit test" in production code (that's why i called the null check without a plan B naive).

Looking at wikibase.ui.entityViewInit, I wonder if wikibase.entityPage.entityLoaded can really be called its dependency - it's a script which triggers a hook. Making the firing of the hook itself a second class citizen (depend on something else) is kind of a double conditional. I think the hook should be a first class citizen (should fire when ever it is possible, i.e. FullEntityParserOutputGenerator) and who ever fancies should react to it (or not) how ever they please. Or in other words: GUI modules may rely on the hook to fire, but the important hook should not rely on some of our GUI module to have been used - and, from my point of view, RL is not right way to model that relationship.

What do you think (conceptually)?

Change 589306 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] entityLoaded: fire depending on relevant pages, not other modules

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

Change 589306 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] entityLoaded: fire depending on relevant pages, not other modules

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

Michael closed this task as Resolved.Mon, Jun 22, 10:08 AM
Michael claimed this task.
Michael added a subscriber: Michael.

As far as I can tell, this has been deployed with last weeks train and we didn't see any related errors. Closing this as fixed 👍

Restricted Application added a project: User-Michael. · View Herald TranscriptMon, Jun 22, 10:08 AM