Page MenuHomePhabricator

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


Seen on ConfirmEdit (CAPTCHA extension) patch

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
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 count'
18:23:41   testrunner
18:23:41     ✖️ Setup
18:23:41 WARN: 'Pending requests does not match count'
18:23:41     ✖️ Teardown
18:23:41 WARN: 'Pending requests does not match count'
18:23:41     ✖️ Loader status
18:23:41 WARN: 'Pending requests does not match count'
18:23:41     ✖️ assert.htmlEqual
18:23:41 WARN: 'Pending requests does not match count'
18:23:41   testrunner-after
18:23:41     ✖️ Teardown

Event Timeline

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

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?

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

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

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 👍