Page MenuHomePhabricator

mw.language unittest cleanup failing
Closed, ResolvedPublic

Description

On https://gerrit.wikimedia.org/r/#/c/325177/ mwext-qunit-jessie fails with mw.language.convertNumber( 12.34 ) returning 12,34. Debugging shows that the separator conversion table for English is {'.': ',', ',': '.'} which probably comes from the mw.language unit tests. The errors are nondeterministic on my local machine - about half the time the conversion table is undefined and all works as it should.

The mw.language test cleanup uses mw.Map.values which has been deprecated in T146432. For some reason I don't see any deprecation warning on the console. Maybe it causes issues somehow? (although I can't imagine how it could).

On a recheck a different test fails still related to comma vs dot as a decimal separator:

mmv.ui.metadataPanel Setting location information works as expected FAILED
Location text is set as expected - if this fails it may be due to i18n issues.

                              vvv               vvv
Expected: "Location: 12° 20′ 44.44″ N, 98° 45′ 55.56″ E"
Actual:   "Location: 12° 20′ 44,44″ N, 98° 45′ 55,56″ E"
                              ^^^               ^^^
                               |                 |

Event Timeline

Tgr created this task.Dec 6 2016, 2:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 6 2016, 2:18 AM
Tgr added a subscriber: hashar.Dec 6 2016, 7:24 AM
hashar updated the task description. (Show Details)Dec 9 2016, 11:05 AM

This looks pretty similar to what we have found as a cause of RevisionSlider tests failures started around last weekend (T153121). We've managed to track the issue down to https://gerrit.wikimedia.org/r/#/c/324387/.

mw.language.convertNumber test added there is changing separatorTransformTable to an arbitrary value, and does not re-set it after the test, which causes the setting to be applied to tests run after that one. This affects the way numbers are printed, as seen in this case too.

What we've done regarding T153121 was setting separatorTransformTable to null at the beginning of number-printing-senstive tests, so all changes from mw.language.convertNumber test do not affect "our" tests. We also remember the original value before "our" test and restore it after it is finished, just in case further tests relied on this specific value of separatorTransformTable (this does not seem to be a case, though, but I only had a very brief look).

Setting that field to null would probably be a quick solution. But the question that also came to my mind yesterday was is it actually intended mw.language qunit test change some settings but do not "clean up" after themselves? This separatorTransformTable issue is only related to a single test but for example wgUserLanguage is being changed multiple times in these teste. Can we be sure those settings do not affect other tests? Especially what I was thinking was maybe their order guarantees that language/locale setting do not affect stability of core qunit tests suites but could we be that sure about this when it comes to extensions?

There are arguments for both ways: tests should define their dependencies and avoid global state; tests should clean up after themselves (if the framework does not do it).

Resetting default values seems simple and sane there.

Tgr added a comment.Dec 15 2016, 10:19 PM

It does reset them though: the teardown callback resets mw.language.data.values to what was saved in the setup. As far as I can see this includes separatorTransformTable. The problem is that for some reason it nondeterministically fails to have any effect.

Krinkle added a comment.EditedDec 17 2016, 12:56 AM

The mw.language test cleanup uses mw.Map.values which has been deprecated

Actually, it doesn't. mw.language.data is not a mw.Map instance and the setup/teardown code in mediawiki.language.test.js effectively does nothing - other than storing the undefined value, creating an unexpected values data property (as language code) and setting it to an empty object.

tests/.../mediawiki.language.test.js
QUnit.module( 'mediawiki.language', QUnit.newMwEnvironment( {
  setup: function () {
    this.liveLangData = mw.language.data.values;
    mw.language.data.values = $.extend( true, {}, this.liveLangData );
  },
  teardown: function () {
     mw.language.data.values = this.liveLangData;
src/.../mediawiki.language.init.js
	/** ..
	 * @class
	 * @singleton
	 */
	mw.language = {
		/** ..
		 * @property
		 */
		data: {},

Fixed in https://gerrit.wikimedia.org/r/327881 - though I suspect it may be unrelated to this task's bug.

Tgr closed this task as Resolved.Dec 17 2016, 5:11 AM
Tgr assigned this task to Krinkle.

Seems like that fixed the issue. Thanks!