From 93f1c9f79e7e5ba3dfd9245cbd2e99eee68cdb86 Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Thu, 11 May 2017 17:03:52 +0200 Subject: [PATCH] Fix mmv qunit tests This heavily relied on deferreds getting resolved synchroneously, which (for .then) is no longer the case with jQuery 3. There's also a difference in how chained .then's get resolved, and $.when no longer propagates notify(). The changes in here are basically: * fix use of $.when, manually passing along notify() * use .then in some places, instead of .done, .fail, .progress * fix progress bar hiding in setupProgressBar, which assumed sync * fixed tests, mostly by using fake timers to give async stuff a chance to execute Bug: T164473 Change-Id: Ib51ddd8bc6254a477861588fb16f57565353afe1 --- resources/mmv/mmv.js | 180 +++++++++++++++++++++--------------- tests/qunit/mmv/mmv.test.js | 97 ++++++++++++------- 2 files changed, 168 insertions(+), 109 deletions(-) diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index f7b2eeedd..d6aa6f4a2 100644 --- a/resources/mmv/mmv.js +++ b/resources/mmv/mmv.js @@ -288,63 +288,78 @@ metadataPromise = this.fetchSizeIndependentLightboxInfo( image.filePageTitle ); - imagePromise.done( function ( thumbnail, imageElement ) { - if ( viewer.currentIndex !== image.index ) { - return; - } + imagePromise.then( + // done + function ( thumbnail, imageElement ) { + if ( viewer.currentIndex !== image.index ) { + return; + } - if ( viewer.imageDisplayedCount++ === 0 ) { - mw.mmv.durationLogger.stop( 'click-to-first-image' ); + if ( viewer.imageDisplayedCount++ === 0 ) { + mw.mmv.durationLogger.stop( 'click-to-first-image' ); - metadataPromise.done( function ( imageInfo ) { - if ( !imageInfo || !imageInfo.anonymizedUploadDateTime ) { - return; - } + metadataPromise.then( function ( imageInfo, repoInfo ) { + if ( imageInfo && imageInfo.anonymizedUploadDateTime ) { + mw.mmv.durationLogger.record( 'click-to-first-image', { + uploadTimestamp: imageInfo.anonymizedUploadDateTime + } ); + } - mw.mmv.durationLogger.record( 'click-to-first-image', { - uploadTimestamp: imageInfo.anonymizedUploadDateTime + return $.Deferred().resolve( imageInfo, repoInfo ); } ); + } + + imageElement.className = 'mw-mmv-final-image ' + image.filePageTitle.ext.toLowerCase(); + imageElement.alt = image.alt; + + $.when( metadataPromise, pluginsPromise ).then( function ( metadata ) { + $( document ).trigger( $.Event( 'mmv-metadata', { viewer: viewer, image: image, imageInfo: metadata[ 0 ] } ) ); } ); + + viewer.displayRealThumbnail( thumbnail, imageElement, imageWidths, $.now() - start ); + + return $.Deferred().resolve( thumbnail, imageElement ); + }, + // fail + function ( error ) { + viewer.ui.canvas.showError( error ); + return $.Deferred().reject( error ); } + ); - imageElement.className = 'mw-mmv-final-image ' + image.filePageTitle.ext.toLowerCase(); - imageElement.alt = image.alt; + metadataPromise.then( + // done + function ( imageInfo, repoInfo ) { + extraStatsDeferred.resolve( { uploadTimestamp: imageInfo.anonymizedUploadDateTime } ); - $.when( metadataPromise, pluginsPromise ).done( function ( metadata ) { - $( document ).trigger( $.Event( 'mmv-metadata', { viewer: viewer, image: image, imageInfo: metadata[ 0 ] } ) ); - } ); + if ( viewer.currentIndex !== image.index ) { + return; + } - viewer.displayRealThumbnail( thumbnail, imageElement, imageWidths, $.now() - start ); - } ).fail( function ( error ) { - viewer.ui.canvas.showError( error ); - } ); + if ( viewer.metadataDisplayedCount++ === 0 ) { + mw.mmv.durationLogger.stop( 'click-to-first-metadata' ).record( 'click-to-first-metadata' ); + } - metadataPromise.done( function ( imageInfo, repoInfo ) { - extraStatsDeferred.resolve( { uploadTimestamp: imageInfo.anonymizedUploadDateTime } ); + viewer.ui.panel.setImageInfo( image, imageInfo, repoInfo ); - if ( viewer.currentIndex !== image.index ) { - return; + // File reuse steals a bunch of information from the DOM, so do it last + viewer.ui.setFileReuseData( imageInfo, repoInfo, image.caption, image.alt ); + + return $.Deferred().resolve( imageInfo, repoInfo ); + }, + // fail + function ( error ) { + extraStatsDeferred.reject(); + + if ( viewer.currentIndex === image.index ) { + // Set title to caption or file name if caption is not available; + // see setTitle() in mmv.ui.metadataPanel for extended caption fallback + viewer.ui.panel.showError( image.caption || image.filePageTitle.getNameText(), error ); + } + + return $.Deferred().reject( error ); } - - if ( viewer.metadataDisplayedCount++ === 0 ) { - mw.mmv.durationLogger.stop( 'click-to-first-metadata' ).record( 'click-to-first-metadata' ); - } - - viewer.ui.panel.setImageInfo( image, imageInfo, repoInfo ); - - // File reuse steals a bunch of information from the DOM, so do it last - viewer.ui.setFileReuseData( imageInfo, repoInfo, image.caption, image.alt ); - } ).fail( function ( error ) { - extraStatsDeferred.reject(); - - if ( viewer.currentIndex !== image.index ) { - return; - } - - // Set title to caption or file name if caption is not available; - // see setTitle() in mmv.ui.metadataPanel for extended caption fallback - viewer.ui.panel.showError( image.caption || image.filePageTitle.getNameText(), error ); - } ); + ); $.when( imagePromise, metadataPromise ).then( function () { if ( viewer.currentIndex !== image.index ) { @@ -507,12 +522,6 @@ progressBar = viewer.ui.panel.progressBar, key = image.filePageTitle.getPrefixedDb() + '|' + imageWidth; - if ( imagePromise.state() !== 'pending' ) { - // image has already loaded (or failed to load) - do not show the progress bar - progressBar.hide(); - return; - } - if ( !this.progressCache[ key ] ) { // Animate progress bar to 5 to give a sense that something is happening, and make sure // the progress bar is noticeable, even if we're sitting at 0% stuck waiting for @@ -525,34 +534,47 @@ } // FIXME would be nice to have a "filtered" promise which does not fire when the image is not visible - imagePromise.progress( function ( progress ) { - // We pretend progress is always at least 5%, so progress events below 5% should be ignored - // 100 will be handled by the done handler, do not mix two animations - if ( progress < 5 || progress === 100 ) { - return; - } + imagePromise.then( + // done + function ( thumbnail, imageElement ) { + viewer.progressCache[ key ] = 100; + if ( viewer.currentIndex === image.index ) { + // Fallback in case the browser doesn't have fancy progress updates + progressBar.animateTo( 100 ); - viewer.progressCache[ key ] = progress; + // Hide progress bar, we're done + progressBar.hide(); + } - // Touch the UI only if the user is looking at this image - if ( viewer.currentIndex === image.index ) { - progressBar.animateTo( progress ); - } - } ).done( function () { - viewer.progressCache[ key ] = 100; + return $.Deferred().resolve( thumbnail, imageElement ); + }, + // fail + function ( error ) { + viewer.progressCache[ key ] = 100; - if ( viewer.currentIndex === image.index ) { - // Fallback in case the browser doesn't have fancy progress updates - progressBar.animateTo( 100 ); - } - } ).fail( function () { - viewer.progressCache[ key ] = 100; + if ( viewer.currentIndex === image.index ) { + // Hide progress bar on error + progressBar.hide(); + } - if ( viewer.currentIndex === image.index ) { - // Hide progress bar on error - progressBar.hide(); + return $.Deferred().reject( error ); + }, + // progress + function ( progress ) { + // We pretend progress is always at least 5%, so progress events below 5% should be ignored + // 100 will be handled by the done handler, do not mix two animations + if ( progress >= 5 && progress < 100 ) { + viewer.progressCache[ key ] = progress; + + // Touch the UI only if the user is looking at this image + if ( viewer.currentIndex === image.index ) { + progressBar.animateTo( progress ); + } + } + + return progress; } - } ); + ); }; /** @@ -762,6 +784,7 @@ MMVP.fetchThumbnail = function ( fileTitle, width, sampleUrl, originalWidth, originalHeight, extraStatsDeferred ) { var viewer = this, guessing = false, + combinedDeferred = $.Deferred(), thumbnailPromise, imagePromise; @@ -806,10 +829,13 @@ } ); } - return $.when( thumbnailPromise, imagePromise ).then( null, null, function ( thumbnailProgress, imageProgress ) { - // Make progress events have a nice format. - return imageProgress[ 1 ]; + // In jQuery<3, $.when used to also relay notify, but that is no longer + // the case - but we still want to pass it along... + $.when( thumbnailPromise, imagePromise ).then( combinedDeferred.resolve, combinedDeferred.reject ); + imagePromise.then( null, null, function ( arg, progress ) { + combinedDeferred.notify( progress ); } ); + return combinedDeferred; }; /** diff --git a/tests/qunit/mmv/mmv.test.js b/tests/qunit/mmv/mmv.test.js index b5a0cbdfe..0e03a368d 100644 --- a/tests/qunit/mmv/mmv.test.js +++ b/tests/qunit/mmv/mmv.test.js @@ -117,7 +117,9 @@ fakeImage = { filePageTitle: new mw.Title( 'File:Stuff.jpg' ), extraStatsDeferred: $.Deferred().reject() - }; + }, + // custom clock ensures progress handlers execute in correct sequence + clock = this.sandbox.useFakeTimers(); viewer.thumbs = []; viewer.displayPlaceholderThumbnail = $.noop; @@ -150,21 +152,27 @@ viewer.imageInfoProvider.get = function () { return $.Deferred().resolve( {} ); }; viewer.thumbnailInfoProvider.get = function () { return $.Deferred().resolve( {} ); }; + // loadImage will call setupProgressBar, which will attach done, fail & + // progress handlers viewer.loadImage( fakeImage, new Image() ); + clock.tick( 10 ); assert.ok( viewer.ui.panel.progressBar.jumpTo.lastCall.calledWith( 0 ), 'Percentage correctly reset by loadImage' ); - - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 5 ), + assert.ok( viewer.ui.panel.progressBar.animateTo.firstCall.calledWith( 5 ), 'Percentage correctly animated to 5 by loadImage' ); imageDeferred.notify( 'response', 45 ); - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 45 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.secondCall.calledWith( 45 ), 'Percentage correctly funneled to panel UI' ); imageDeferred.resolve( {}, {} ); - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 100 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.thirdCall.calledWith( 100 ), 'Percentage correctly funneled to panel UI' ); + clock.restore(); + viewer.close(); } ); @@ -181,7 +189,9 @@ filePageTitle: new mw.Title( 'File:Second.jpg' ), extraStatsDeferred: $.Deferred().reject() }, - viewer = mw.mmv.testHelpers.getMultimediaViewer(); + viewer = mw.mmv.testHelpers.getMultimediaViewer(), + // custom clock ensures progress handlers execute in correct sequence + clock = this.sandbox.useFakeTimers(); // animation would keep running, conflict with other tests this.sandbox.stub( $.fn, 'animate' ).returnsThis(); @@ -223,60 +233,65 @@ // load some image viewer.imageProvider.get = this.sandbox.stub().returns( firstImageDeferred ); viewer.loadImage( firstImage, new Image() ); - - assert.ok( viewer.ui.panel.progressBar.jumpTo.lastCall.calledWith( 0 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.jumpTo.getCall( 0 ).calledWith( 0 ), 'Percentage correctly reset for new first image' ); - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 5 ), + assert.ok( viewer.ui.panel.progressBar.animateTo.getCall( 0 ).calledWith( 5 ), 'Percentage correctly animated to 5 for first new image' ); + // progress on active image firstImageDeferred.notify( 'response', 20 ); - - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 20 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.getCall( 1 ).calledWith( 20 ), 'Percentage correctly animated when active image is loading' ); // change to another image viewer.imageProvider.get = this.sandbox.stub().returns( secondImageDeferred ); viewer.loadImage( secondImage, new Image() ); - - assert.ok( viewer.ui.panel.progressBar.jumpTo.lastCall.calledWith( 0 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.jumpTo.getCall( 1 ).calledWith( 0 ), 'Percentage correctly reset for second new image' ); - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 5 ), + assert.ok( viewer.ui.panel.progressBar.animateTo.getCall( 2 ).calledWith( 5 ), 'Percentage correctly animated to 5 for second new image' ); + // progress on active image secondImageDeferred.notify( 'response', 30 ); - - assert.ok( viewer.ui.panel.progressBar.animateTo.lastCall.calledWith( 30 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.getCall( 3 ).calledWith( 30 ), 'Percentage correctly animated when active image is loading' ); - // this is the most convenient way of checking for new calls - just reset() and check called - viewer.ui.panel.progressBar.animateTo.reset(); - viewer.ui.panel.progressBar.jumpTo.reset(); - + // progress on inactive image firstImageDeferred.notify( 'response', 40 ); - - assert.ok( !viewer.ui.panel.progressBar.animateTo.called, + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.callCount === 4, 'Percentage not animated when inactive image is loading' ); - assert.ok( !viewer.ui.panel.progressBar.jumpTo.called, - 'Percentage not changed when inactive image is loading' ); + // progress on active image secondImageDeferred.notify( 'response', 50 ); + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.animateTo.getCall( 4 ).calledWith( 50 ), + 'Percentage correctly ignored inactive image & only animated when active image is loading' ); // change back to first image + viewer.imageProvider.get = this.sandbox.stub().returns( firstImageDeferred ); viewer.loadImage( firstImage, new Image() ); - - assert.ok( viewer.ui.panel.progressBar.jumpTo.lastCall.calledWith( 40 ), + clock.tick( 10 ); + assert.ok( viewer.ui.panel.progressBar.jumpTo.getCall( 2 ).calledWith( 40 ), 'Percentage jumps to right value when changing images' ); secondImageDeferred.resolve( {}, {} ); + clock.tick( 10 ); assert.ok( !viewer.ui.panel.progressBar.hide.called, 'Progress bar not hidden when something finishes in the background' ); - // change to second image which has finished loading + // change back to second image, which has finished loading viewer.imageProvider.get = this.sandbox.stub().returns( secondImageDeferred ); viewer.loadImage( secondImage, new Image() ); - + clock.tick( 10 ); assert.ok( viewer.ui.panel.progressBar.hide.called, - 'Progress bar not hidden when switching to finished image' ); + 'Progress bar hidden when switching to finished image' ); + + clock.restore(); viewer.close(); } ); @@ -400,7 +415,9 @@ filePageTitle: new mw.Title( 'File:Bar.jpg' ), index: 1, extraStatsDeferred: $.Deferred().reject() - }; + }, + // custom clock ensures progress handlers execute in correct sequence + clock = this.sandbox.useFakeTimers(); viewer.preloadFullscreenThumbnail = $.noop; viewer.fetchSizeIndependentLightboxInfo = this.sandbox.stub(); @@ -435,25 +452,32 @@ viewer.imageProvider.get.returns( firstImageDeferred.promise() ); viewer.fetchSizeIndependentLightboxInfo.returns( firstLigthboxInfoDeferred.promise() ); viewer.loadImage( firstImage, new Image() ); + clock.tick( 10 ); assert.ok( !viewer.animateMetadataDivOnce.called, 'Metadata of the first image should not be animated' ); assert.ok( !viewer.ui.panel.setImageInfo.called, 'Metadata of the first image should not be shown' ); viewer.imageProvider.get.returns( secondImageDeferred.promise() ); viewer.fetchSizeIndependentLightboxInfo.returns( secondLigthboxInfoDeferred.promise() ); viewer.loadImage( secondImage, new Image() ); + clock.tick( 10 ); viewer.ui.panel.progressBar.animateTo.reset(); firstImageDeferred.notify( undefined, 45 ); + clock.tick( 10 ); assert.ok( !viewer.ui.panel.progressBar.animateTo.reset.called, 'Progress of the first image should not be shown' ); firstImageDeferred.resolve( {}, {} ); firstLigthboxInfoDeferred.resolve( {} ); + clock.tick( 10 ); assert.ok( !viewer.displayRealThumbnail.called, 'The first image being done loading should have no effect' ); viewer.displayRealThumbnail = this.sandbox.spy( function () { viewer.close(); } ); secondImageDeferred.resolve( {}, {} ); secondLigthboxInfoDeferred.resolve( {} ); + clock.tick( 10 ); assert.ok( viewer.displayRealThumbnail.called, 'The second image being done loading should result in the image being shown' ); + + clock.restore(); } ); QUnit.test( 'Events are not trapped after the viewer is closed', 0, function ( assert ) { @@ -558,7 +582,9 @@ width = 100, originalWidth = 1000, originalHeight = 1000, - image = {}; + image = {}, + // custom clock ensures progress handlers execute in correct sequence + clock = this.sandbox.useFakeTimers(); function setupStubs() { guessedThumbnailInfoStub = viewer.guessedThumbnailInfoProvider.get = sandbox.stub(); @@ -574,6 +600,7 @@ thumbnailInfoStub.returns( $.Deferred().resolve( { url: 'apiURL' } ) ); imageStub.returns( $.Deferred().resolve( image ) ); promise = viewer.fetchThumbnail( file, width ); + clock.tick( 10 ); assert.ok( !guessedThumbnailInfoStub.called, 'When we lack sample URL and original dimensions, GuessedThumbnailInfoProvider is not called' ); assert.ok( thumbnailInfoStub.calledOnce, 'When we lack sample URL and original dimensions, ThumbnailInfoProvider is called once' ); assert.ok( imageStub.calledOnce, 'When we lack sample URL and original dimensions, ImageProvider is called once' ); @@ -586,6 +613,7 @@ thumbnailInfoStub.returns( $.Deferred().resolve( { url: 'apiURL' } ) ); imageStub.returns( $.Deferred().resolve( image ) ); promise = viewer.fetchThumbnail( file, width, sampleURL, originalWidth, originalHeight ); + clock.tick( 10 ); assert.ok( guessedThumbnailInfoStub.calledOnce, 'When the guesser bails out, GuessedThumbnailInfoProvider is called once' ); assert.ok( thumbnailInfoStub.calledOnce, 'When the guesser bails out, ThumbnailInfoProvider is called once' ); assert.ok( imageStub.calledOnce, 'When the guesser bails out, ImageProvider is called once' ); @@ -598,6 +626,7 @@ thumbnailInfoStub.returns( $.Deferred().resolve( { url: 'apiURL' } ) ); imageStub.returns( $.Deferred().resolve( image ) ); promise = viewer.fetchThumbnail( file, width, sampleURL, originalWidth, originalHeight ); + clock.tick( 10 ); assert.ok( guessedThumbnailInfoStub.calledOnce, 'When the guesser returns an URL, GuessedThumbnailInfoProvider is called once' ); assert.ok( !thumbnailInfoStub.called, 'When the guesser returns an URL, ThumbnailInfoProvider is not called' ); assert.ok( imageStub.calledOnce, 'When the guesser returns an URL, ImageProvider is called once' ); @@ -611,6 +640,7 @@ imageStub.withArgs( 'guessedURL' ).returns( $.Deferred().reject() ); imageStub.withArgs( 'apiURL' ).returns( $.Deferred().resolve( image ) ); promise = viewer.fetchThumbnail( file, width, sampleURL, originalWidth, originalHeight ); + clock.tick( 10 ); assert.ok( guessedThumbnailInfoStub.calledOnce, 'When the guesser returns an URL, but that returns 404, GuessedThumbnailInfoProvider is called once' ); assert.ok( thumbnailInfoStub.calledOnce, 'When the guesser returns an URL, but that returns 404, ThumbnailInfoProvider is called once' ); assert.ok( imageStub.calledTwice, 'When the guesser returns an URL, but that returns 404, ImageProvider is called twice' ); @@ -625,6 +655,7 @@ imageStub.withArgs( 'guessedURL' ).returns( $.Deferred().reject() ); imageStub.withArgs( 'apiURL' ).returns( $.Deferred().reject() ); promise = viewer.fetchThumbnail( file, width, sampleURL, originalWidth, originalHeight ); + clock.tick( 10 ); assert.ok( guessedThumbnailInfoStub.calledOnce, 'When even the retry fails, GuessedThumbnailInfoProvider is called once' ); assert.ok( thumbnailInfoStub.calledOnce, 'When even the retry fails, ThumbnailInfoProvider is called once' ); assert.ok( imageStub.calledTwice, 'When even the retry fails, ImageProvider is called twice' ); @@ -640,11 +671,14 @@ thumbnailInfoStub.returns( $.Deferred().resolve( { url: 'apiURL' } ) ); imageStub.returns( $.Deferred().resolve( image ) ); promise = viewer.fetchThumbnail( file, width ); + clock.tick( 10 ); assert.ok( !guessedThumbnailInfoStub.called, 'When guessing is disabled, GuessedThumbnailInfoProvider is not called' ); assert.ok( thumbnailInfoStub.calledOnce, 'When guessing is disabled, ThumbnailInfoProvider is called once' ); assert.ok( imageStub.calledOnce, 'When guessing is disabled, ImageProvider is called once' ); assert.ok( imageStub.calledWith( 'apiURL' ), 'When guessing is disabled, ImageProvider is called with the API url' ); assert.strictEqual( promise.state(), 'resolved', 'When guessing is disabled, fetchThumbnail resolves' ); + + clock.restore(); } ); QUnit.test( 'document.title', 2, function ( assert ) { @@ -664,5 +698,4 @@ assert.strictEqual( document.title, oldDocumentTitle, 'Original title restored after viewer is closed' ); } ); - }( mediaWiki, jQuery ) );