From b90bc351e3cd8d1e5e0dd7fe6f340c770201c06e Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 28 Feb 2018 19:28:42 -0800
Subject: [PATCH] resourceloader: Fix broken code in load.php mock used to make
 a test fail

The load.mock.php has some module implementation stubs that call global
QUnit methods. This has been broken since the upgrade to QUnit 2, which removed
support for these (deprecated) methods, in favour of context-based assert.

However, this went unnoticed because these stubs are only there to make
our test fail if code is behaving incorrectly. Naturally, this isn't the
case normally. In theory, any unmerged code between QUnit 2 upgrade and now
would've failed Jenkins for something like 'QUnit.ok undefined' instead of
the more descriptive error message the stub supplies.

Fix this by instead statically exposing the contextual assert object
to the stubs.

Also centralise the clean up (teardown) for this new exposure, and also
clean up the other static exposure we have (testCallback) in the same way,
and document the cases where we intentionally clean it up inline.

Change-Id: I00b71e7bc9aa97275dfabf1070c4141fa76adb05
---
 tests/qunit/data/load.mock.php                |  8 +++----
 .../mediawiki/mediawiki.loader.test.js        | 22 ++++++++++++++-----
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/tests/qunit/data/load.mock.php b/tests/qunit/data/load.mock.php
index 23fb4c912c..fed5746c02 100644
--- a/tests/qunit/data/load.mock.php
+++ b/tests/qunit/data/load.mock.php
@@ -27,21 +27,19 @@ header( 'Content-Type: text/javascript; charset=utf-8' );
 $moduleImplementations = [
 	'testUsesMissing' => "
 mw.loader.implement( 'testUsesMissing', function () {
-	QUnit.ok( false, 'Module usesMissing script should not run.' );
-	QUnit.start();
+	mw.loader.testFail( 'Module usesMissing script should not run.' );
 }, {}, {});
 ",
 
 	'testUsesNestedMissing' => "
 mw.loader.implement( 'testUsesNestedMissing', function () {
-	QUnit.ok( false, 'Module testUsesNestedMissing script should not run.' );
-	QUnit.start();
+	mw.loader.testFail('Module testUsesNestedMissing script should not run.' );
 }, {}, {});
 ",
 
 	'testSkipped' => "
 mw.loader.implement( 'testSkipped', function () {
-	QUnit.ok( false, 'Module testSkipped was supposed to be skipped.' );
+	mw.loader.testFail( false, 'Module testSkipped was supposed to be skipped.' );
 }, {}, {});
 ",
 
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
index 64415e0294..e774112188 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
@@ -1,7 +1,12 @@
 ( function ( mw, $ ) {
 	QUnit.module( 'mediawiki (mw.loader)', QUnit.newMwEnvironment( {
-		setup: function () {
+		setup: function ( assert ) {
 			mw.loader.store.enabled = false;
+
+			// Expose for load.mock.php
+			mw.loader.testFail = function ( reason ) {
+				assert.ok( false, reason );
+			};
 		},
 		teardown: function () {
 			mw.loader.store.enabled = false;
@@ -10,6 +15,14 @@
 				window.Set = this.nativeSet;
 				mw.redefineFallbacksForTest();
 			}
+			// Remove any remaining temporary statics
+			// exposed for cross-file mocks.
+			if ( 'testCallback' in mw.loader ) {
+				delete mw.loader.testCallback;
+			}
+			if ( 'testFail' in mw.loader ) {
+				delete mw.loader.testFail;
+			}
 		}
 	} ) );
 
@@ -94,8 +107,6 @@
 
 		return mw.loader.using( 'test.callback', function () {
 			assert.strictEqual( isAwesomeDone, true, 'test.callback module should\'ve caused isAwesomeDone to be true' );
-			delete mw.loader.testCallback;
-
 		}, function () {
 			assert.ok( false, 'Error callback fired while loader.using "test.callback" module' );
 		} );
@@ -113,8 +124,6 @@
 
 		return mw.loader.using( 'hasOwnProperty', function () {
 			assert.strictEqual( isAwesomeDone, true, 'hasOwnProperty module should\'ve caused isAwesomeDone to be true' );
-			delete mw.loader.testCallback;
-
 		}, function () {
 			assert.ok( false, 'Error callback fired while loader.using "hasOwnProperty" module' );
 		} );
@@ -133,7 +142,6 @@
 		return mw.loader.using( 'test.promise' )
 			.done( function () {
 				assert.strictEqual( isAwesomeDone, true, 'test.promise module should\'ve caused isAwesomeDone to be true' );
-				delete mw.loader.testCallback;
 			} )
 			.fail( function () {
 				assert.ok( false, 'Error callback fired while loader.using "test.promise" module' );
@@ -710,6 +718,7 @@
 		assert.equal( target.slice( 0, 2 ), '//', 'URL is protocol-relative' );
 
 		mw.loader.testCallback = function () {
+			// Ensure once, delete now
 			delete mw.loader.testCallback;
 			assert.ok( true, 'callback' );
 			done();
@@ -728,6 +737,7 @@
 		assert.equal( target.slice( 0, 1 ), '/', 'URL is relative to document root' );
 
 		mw.loader.testCallback = function () {
+			// Ensure once, delete now
 			delete mw.loader.testCallback;
 			assert.ok( true, 'callback' );
 			done();
-- 
2.20.1