Page MenuHomePhabricator

ApiQueryLanguageInfoTest uses ConvertibleTimestamp::setFakeTime incorrectly
Closed, ResolvedPublic

Description

As of 932f9c3eb, ApiQueryLanguageInfoTest calls ConvertibleTimestamp::setFakeTime in the following way:

$time = 0;
ConvertibleTimestamp::setFakeTime( static function () use ( &$time ) {
	return $time += 0.75;
} );

CI does not complain, but ConvertibleTimestamp::setFakeTime clearly documents that the parameter passed to setFakeTime can be "a callback() returning an int representing a UNIX epoch". Up to v4.0.0 of wikimedia/timestamp, ConvertibleTimestamp didn't enforce that documented expectation. In R1985:bbba1bc9ab46: setFakeTime: add $step parameter, @daniel changed MediaWiki-libs-Timestamp in a way that casts the callback's output to an int, breaking ApiQueryLanguageInfoTest.

The magic numbers of 0.75 and 1.5 were introduced by @Lucas_Werkmeister_WMDE in rMW67b3cdc0047f: Add action=query&meta=languageinfo API module (without calling setFakeTime though). In rMW9b41241cc88e: api: Use fake timer to test ApiQueryLanguageinfo, @Umherirrender started using setFakeTime, effectively causing this inconsistency.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 889064 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/libs/Timestamp@master] Do not cast fake time callable to int

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

Uploading a patch to remove the cast (so we return back to making use of an undocumentated behavior), to help me unblock getting 09b22e46 in a MW-usable release. Locally, removing the cast fixes the problematic test.

IMHO it would be better to fix the test. ApiQueryLanguageinfo applies continuation if the execution takes more than 2 seconds (edit: an approach suggested by Anomie in T217239#4994301), and so I think the numbers 0.75 and 1.5 were chosen so that repeated time() calls would either stay below this limit or exceed it – there aren’t many integers to choose from below 2 ;) but I think a better approach would be to bump this limit a bit and then change the test to use ints – either hard-code a slightly higher AapiQueryLanguageinfo::MAX_EXECUTE_SECONDS constant (how about 5?), or add a config setting for it that the test can change. (IMHO, hard-coding 5 instead of 2 should be good enough and a config variable feels like unnecessary complexity.)

Change 889115 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/core@master] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

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

IMHO it would be better to fix the test. ApiQueryLanguageinfo applies continuation if the execution takes more than 2 seconds, and so I think the numbers 0.75 and 1.5 were chosen so that repeated time() calls would either stay below this limit or exceed it – there aren’t many integers to choose from below 2 ;) but I think a better approach would be to bump this limit a bit and then change the test to use ints – either hard-code a slightly higher AapiQueryLanguageinfo::MAX_EXECUTE_SECONDS constant (how about 5?), or add a config setting for it that the test can change. (IMHO, hard-coding 5 instead of 2 should be good enough and a config variable feels like unnecessary complexity.)

Thank you very much for this explanation! I struggled understanding why only 0.75 and 1.5 worked, and rounding to 1 and 2 didn't. Uploaded an actual fix.

Change 889064 abandoned by Urbanecm:

[mediawiki/libs/Timestamp@master] Do not cast fake time callable to int

Reason:

replaced with I82d49a5e3a52c14dbd7e5324eb4c75cf7c33f3d2

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

Found another comment on Gerrit:

I probably wouldn't go over 5 seconds though.

The linked change only goes for 3 seconds, so I think that should be fine.

Change 889115 merged by jenkins-bot:

[mediawiki/core@master] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

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

Change 889234 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/libs/Timestamp@master] phan: Type hint ConvertibleTimestamp::setFakeTime()

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

Change 889234 merged by jenkins-bot:

[mediawiki/libs/Timestamp@master] phan: Type hint ConvertibleTimestamp::setFakeTime()

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

Change 963812 had a related patch set uploaded (by Jforrester; author: Urbanecm):

[mediawiki/core@REL1_39] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

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

Change 963813 had a related patch set uploaded (by Reedy; author: Urbanecm):

[mediawiki/core@REL1_39] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

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

Change 963813 abandoned by Reedy:

[mediawiki/core@REL1_39] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

Reason:

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

Change 963812 merged by jenkins-bot:

[mediawiki/core@REL1_39] ApiQueryLanguageinfoTest: Do not pass a float to setFakeTime

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