From 5583d10bdc29ee07aa5b5c62af2fb53b49846ae7 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 29 Jun 2012 10:34:12 +0200 Subject: [PATCH] Tests: Fix race-condition broken mw.loader unit test * testloader.php was resolving dependencies when it shouldn't the server *never* does that. That's handled by the client, which knows the state of what is and isn't module. Due to a race condition between: - handlePending() call from loader.state() call for load.phpmodules?=testMissing - asynchronous callback from mw.loader.using as dependencies are being resolved. It didn't expose any actual problem, the unit test was simply assuming that they would arrive in a certain order, when that isn't the case by design. Random failures are seen here: - https://integration.mediawiki.org/testswarm/user/mediawiki/ - http://integration.wmflabs.org/testswarm/job/4 > Module "test.missing" must have state "missing" > - "missing" > + "loading" -> Because callback was triggered from mw.loader.state() already it didn't get to the implement() call yet. > Module "test.use_missing" must have state "error" > - "error" > + "loading" -> (same reason) This is now fixed. * Greatly simplified the mock load.php Change-Id: I000ee726a062f6c6d630ad6c07cfc0b48d145d35 --- tests/qunit/data/load.mock.php | 58 ++++++++++++++++ tests/qunit/data/testloader.php | 69 ------------------- .../resources/mediawiki/mediawiki.test.js | 55 ++++++++------- 3 files changed, 89 insertions(+), 93 deletions(-) create mode 100644 tests/qunit/data/load.mock.php delete mode 100644 tests/qunit/data/testloader.php diff --git a/tests/qunit/data/load.mock.php b/tests/qunit/data/load.mock.php new file mode 100644 index 0000000000..1c189703ee --- /dev/null +++ b/tests/qunit/data/load.mock.php @@ -0,0 +1,58 @@ + " +mw.loader.implement( 'testUsesMissing', function () { + QUnit.ok( false, 'Module test.usesMissing script should not run.'); + QUnit.start(); +}, {}, {}); +", + + 'testUsesNestedMissing' => " +mw.loader.implement( 'testUsesNestedMissing', function () { + QUnit.ok( false, 'Module testUsesNestedMissing script should not run.'); +}, {}, {}); +", +); + +$response = ''; + +// Only support for non-encoded module names, full module names expected +if ( isset( $_GET['modules'] ) ) { + $modules = explode( ',', $_GET['modules'] ); + foreach ( $modules as $module ) { + if ( isset( $moduleImplementations[$module] ) ) { + $response .= $moduleImplementations[$module]; + } else { + $response .= Xml::encodeJsCall( 'mw.loader.state', array( $module, 'missing' ) ); + } + } +} + +echo $response; diff --git a/tests/qunit/data/testloader.php b/tests/qunit/data/testloader.php deleted file mode 100644 index 967983ede8..0000000000 --- a/tests/qunit/data/testloader.php +++ /dev/null @@ -1,69 +0,0 @@ - array ( - 'src' => 'mw.loader.implement("test.use_missing", function () {start(); ok(false, "Module test.use_missing should not run.");}, {}, {});', - 'deps' => array ( 'test.missing' ) - ), - 'test.use_missing2' => array ( - 'src' => 'mw.loader.implement("test.use_missing2", function () {start(); ok(false, "Module test.use_missing2 should not run.");}, {}, {});', - 'deps' => array ( 'test.missing2' ) - ) -); - -function addModule ( $modules, $moduleName, &$gotten ) { - $result = ""; - if ( isset( $gotten[$moduleName] ) ) { - return $result; - } - $gotten[$moduleName] = true; - if ( isset( $modules[$moduleName] ) ) { - $deps = $modules[$moduleName]['deps']; - foreach ( $deps as $depName ) { - $result .= addModule( $depName, $gotten ) . "\n"; - } - $result .= $modules[$moduleName]['src'] . "\n"; - } else { - $result .= 'mw.loader.state( ' . Xml::encodeJsVar( $moduleName ) . ', "missing" );' . "\n"; - } - return $result . "\n"; -} - -$result = ""; - -if ( isset( $_GET['modules'] ) ) { - $toGet = ResourceLoaderContext::expandModuleNames( $_GET['modules'] ); - $gotten = array(); - foreach ( $toGet as $moduleName ) { - $result .= addModule( $modules, $moduleName, $gotten ); - } -} - -echo $result; diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js index 2f115215af..00fcf38c76 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js @@ -343,43 +343,50 @@ test( 'mw.loader missing dependency', function() { ); } ); -test( 'mw.loader real missing dependency', function() { - expect( 6 ); +test( 'mw.loader dependency handling', function () { + expect( 5 ); mw.loader.addSource( - 'test', - {'loadScript' : QUnit.fixurl( mw.config.get( 'wgScriptPath' ) + '/tests/qunit/data/testloader.php' )} + 'testloader', + { + loadScript: QUnit.fixurl( mw.config.get( 'wgScriptPath' ) + '/tests/qunit/data/load.mock.php' ) + } ); + mw.loader.register( [ - ['test.missing', '0', [], null, 'test'], ['test.missing2', '0', [], null, 'test'], - ['test.use_missing', '0', ['test.missing'], null, 'test'], - ['test.use_missing2', '0', ['test.missing2'], null, 'test'] + // [module, version, dependencies, group, source] + ['testMissing', '1', [], null, 'testloader'], + ['testUsesMissing', '1', ['testMissing'], null, 'testloader'], + ['testUsesNestedMissing', '1', ['testUsesMissing'], null, 'testloader'] ] ); + function verifyModuleStates() { + equal( mw.loader.getState( 'testMissing' ), 'missing', 'Module not known to server must have state "missing"' ); + equal( mw.loader.getState( 'testUsesMissing' ), 'error', 'Module with missing dependency must have state "error"' ); + equal( mw.loader.getState( 'testUsesNestedMissing' ), 'error', 'Module with indirect missing dependency must have state "error"' ); + } + stop(); - // Asynch ahead - mw.loader.load( ['test.use_missing'] ); + mw.loader.using( ['testUsesNestedMissing'], + function () { + ok( false, 'Error handler should be invoked.' ); + ok( true ); // Dummy to reach QUnit expect() - function verifyModuleStates() { - strictEqual( mw.loader.getState( 'test.missing' ), 'missing', 'Module "test.missing" must have state "missing"' ); - strictEqual( mw.loader.getState( 'test.missing2' ), 'missing', 'Module "test.missing2" must have state "missing"' ); - strictEqual( mw.loader.getState( 'test.use_missing' ), 'error', 'Module "test.use_missing" must have state "error"' ); - strictEqual( mw.loader.getState( 'test.use_missing2' ), 'error', 'Module "test.use_missing2" must have state "error"' ); - } + verifyModuleStates(); - mw.loader.using( ['test.use_missing2'], - function() { start(); - ok( false, "Success called wrongly." ); - ok( true , "QUnit expected() count dummy" ); - verifyModuleStates(); }, - function( e, dependencies ) { - start(); - ok( true, "Error handler called correctly." ); - deepEqual( dependencies, ['test.missing2'], "Dependencies correct." ); + function ( e, badmodules ) { + ok( true, 'Error handler should be invoked.' ); + // As soon as server spits out state('testMissing', 'missing'); + // it will bubble up and trigger the error callback. + // Therefor the badmodules array is not testUsesMissing or testUsesNestedMissing. + deepEqual( badmodules, ['testMissing'], 'Bad modules as expected.' ); + verifyModuleStates(); + + start(); } ); } ); -- 2.20.1