Tests: Add unit tests for callback-only animation signatures#5776
Tests: Add unit tests for callback-only animation signatures#5776tommyhgunz14 wants to merge 2 commits intojquery:mainfrom
Conversation
|
Thanks for the PR. Before we review it, please sign our CLA. |
mgol
left a comment
mgol
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please read my comments.
test/unit/effects.js
Outdated
| div.fadeIn( function() { | ||
| assert.ok( true, "fadeIn(callback) invokes the callback" ); | ||
| } ); | ||
| this.clock.tick( 600 ); | ||
|
|
||
| div.fadeOut( function() { | ||
| assert.ok( true, "fadeOut(callback) invokes the callback" ); | ||
| } ); | ||
| this.clock.tick( 600 ); | ||
|
|
||
| div.fadeToggle( function() { | ||
| assert.ok( true, "fadeToggle(callback) invokes the callback" ); | ||
| } ); | ||
| this.clock.tick( 600 ); |
There was a problem hiding this comment.
We should also have assertions that the callback is not called too early. You can do a tick by 500 first, check that it wasn't called and then by 100 and check that it was called.
Or, even better, don't hardcode the number & use jQuery.fx.speeds._default * 0.8, then jQuery.fx.speeds._default * 0.2.
For this to work, you need to change how you attach callbacks. Instead of a normal callback, attach a sinon spy (we're using a sinon sandbox here, see https://sinonjs.org/releases/latest/sandbox/ & the beforeEach hook in this file) and check if it was called or no via Sinon APIs.
Update all the tests this way.
test/unit/effects.js
Outdated
|
|
||
| var div = jQuery( "<div>" ).appendTo( "#qunit-fixture" ); | ||
|
|
||
| // hide(callback) should animate with default duration |
There was a problem hiding this comment.
These comments don't add a lot of value, please remove them.
|
Oh, and in the commit message & PR description reference gh-1738 instead of |
tommyhgunz14
force-pushed
the
test/callback-only-animation
branch
from
35b0cad to
b0e9578
Compare
| fadeToggleCb = this.sandbox.spy(); | ||
|
|
||
| div.fadeIn( fadeInCb ); | ||
| this.clock.tick( jQuery.fx.speeds._default * 0.8 ); |
There was a problem hiding this comment.
These changes are needed in all tests, not just here.
Add tests for show/hide/toggle(callback), slideDown/slideUp/slideToggle(callback), and fadeIn/fadeOut/fadeToggle(callback) to verify that passing only a callback function as the first argument triggers an animated transition with the default duration. These signatures have been supported historically but were never explicitly tested. The callback-only form is documented and should animate with jQuery.fx.speeds._default (400ms) duration rather than toggling visibility immediately. Ref jquerygh-1738 Closes jquerygh-1738
tommyhgunz14
force-pushed
the
test/callback-only-animation
branch
from
b0e9578 to
a29f145
Compare
mgol
left a comment
mgol
left a comment
There was a problem hiding this comment.
Thanks! We could consider abstracting these three tests since they are almost identical, but 3 tests seem still manageable and debugging is a bit easier, so I'm fine with the way it is.
mgol
added
Tests
Discuss in Meeting
mgol
left a comment
mgol
left a comment
There was a problem hiding this comment.
Oh, actually, the tests are failing. Please run them locally and request re-review when they pass.
mgol
removed
the
Discuss in Meeting
The expected assertion counts were incorrect: - show/hide/toggle test: 12 → 9 (3 groups × 3 assertions) - slideDown/slideUp/slideToggle test: 9 → 6 (3 groups × 2 assertions) - fadeIn/fadeOut/fadeToggle test: 9 → 6 (3 groups × 2 assertions) Ref jquerygh-1738
2 participants
Summary
Adds unit tests for the callback-only signatures of animation methods:
show(callback),hide(callback),toggle(callback), and theirslide*andfade*counterparts.Problem
Passing only a callback function (without a duration) to
show(),hide(),toggle(),slideDown(),slideUp(),slideToggle(),fadeIn(),fadeOut(), andfadeToggle()should trigger an animated transition with the default duration (jQuery.fx.speeds._default, 400ms) and invoke the callback on completion.This behavior has been supported historically but was never explicitly tested.
Ref trac-12821
Changes
Added 3 new test cases to
test/unit/effects.jswith 12 total assertions:show/hide/toggle(callback)— verifies callback invocation and correct visibility state after animation completesslideDown/slideUp/slideToggle(callback)— verifies callback invocation for slide animationsfadeIn/fadeOut/fadeToggle(callback)— verifies callback invocation for fade animationsAll tests use the existing
sinon.useFakeTimerspattern and clean up DOM elements after completion.Closes gh-1738
Ref trac-12821