Tests: Add unit tests for callback-only animation signatures by tommyhgunz14 · Pull Request #5776 · jquery/jquery

tommyhgunz14 · 2026-02-18T11:58:31Z

Summary

Adds unit tests for the callback-only signatures of animation methods: show(callback), hide(callback), toggle(callback), and their slide* and fade* counterparts.

Problem

Passing only a callback function (without a duration) to show(), hide(), toggle(), slideDown(), slideUp(), slideToggle(), fadeIn(), fadeOut(), and fadeToggle() 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.js with 12 total assertions:

show/hide/toggle(callback) — verifies callback invocation and correct visibility state after animation completes
slideDown/slideUp/slideToggle(callback) — verifies callback invocation for slide animations
fadeIn/fadeOut/fadeToggle(callback) — verifies callback invocation for fade animations

All tests use the existing sinon.useFakeTimers pattern and clean up DOM elements after completion.

Closes gh-1738
Ref trac-12821

linux-foundation-easycla · 2026-02-18T11:58:39Z

The committers listed above are authorized under a signed CLA.

✅ login: tommyhgunz14 / name: tommyhgunz14 (06a4281, 1116363, 66284df, 8354a79, 9df4eab, a29f145, ed8f1a5)

mgol · 2026-02-18T12:20:09Z

Thanks for the PR. Before we review it, please sign our CLA.

mgol

Thanks for the PR! Please read my comments.

mgol · 2026-02-23T21:38:57Z

Oh, and in the commit message & PR description reference gh-1738 instead of trac-12821. We should use the migrated issues whenever available.

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

mgol

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

Oh, actually, the tests are failing. Please run them locally and request re-review when they pass.

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

tommyhgunz14 · 2026-03-22T10:19:30Z

Hi - this has been outstanding for a while now, should still be good.

mgol

I had a closer look and I've noticed one other issue, see my comment.

Per review feedback, the slideDown/slideUp/slideToggle and fadeIn/fadeOut/fadeToggle callback-only tests now verify the element's visibility state after each animation completes, matching the pattern already used in the show/hide/toggle(callback) test. Ref jquerygh-1738

mgol · 2026-03-31T21:37:44Z

Tests are failing.

tommyhgunz14 · 2026-04-01T15:52:26Z

Yes - the tests are intentionally demonstrating the bug described in gh-1738. I'll sort out a source fix.

mgol · 2026-04-01T16:12:10Z

Hmm, I don't see any bug described in gh-1738, can you point me to what you're describing?

The completion tick needs to account for the animation frame interval (fxInterval = 13ms). A 400ms animation completes at the first 13ms boundary >= 400ms (ceil(400/13) * 13 = 403ms), so ticking only 401ms (_default * 0.2 + 1) falls short. Use _default * 0.2 + fxInterval instead to ensure the animation has completed. Ref jquerygh-1738

tommyhgunz14 · 2026-04-01T20:07:34Z

Ignore me - mixed up with another PR.
Fixed the timer alignment — the completion tick needs to account for the animation frame interval

The tests use :hidden/:visible pseudo-selectors which require jQuery's custom selector module. Gate them with QUnit.jQuerySelectors to skip when running the selector-native build variant. Ref jquerygh-1738

tommyhgunz14 · 2026-04-01T21:44:40Z

test:selector-native was failing because :hidden/:visible aren't valid in native selector builds. Now gated with QUnit.jQuerySelectors so tests skip instead of crash.
This should be all green now..

Replace :visible/:hidden pseudo-selector assertions with .css("display") checks so that the callback-only animation tests run in both the selector and selector-native builds. These are effects tests, not selector tests, so they should not be gated behind QUnit.jQuerySelectors. - :hidden checks now use .css('display') === 'none' - :visible checks now use .css('display') !== 'none' - Tests no longer skip in selector-native build Ref jquerygh-1738

Use strictEqual with the expected "block" value for visible-state assertions rather than the weaker notStrictEqual against "none". All test elements are divs so the computed display after showing is always "block". Ref jquerygh-1738

mgol

Looks good to me, thanks!

I'll leave it open for a while so that other team members have a chance to look at it, but the Easter break is coming, so some delays are possible. I don't expect major hurdles anymore, though.

Add tests for: * show/hide/toggle(callback) * slideDown/slideUp/slideToggle(callback) * 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. Fixes gh-1738 Closes gh-5776 (cherry picked from commit ac83749)

Add tests for: * show/hide/toggle(callback) * slideDown/slideUp/slideToggle(callback) * 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. Fixes jquerygh-1738 Closes jquerygh-5776 (cherry picked from commit ac83749)

mgol requested changes Feb 23, 2026

View reviewed changes

Comment thread test/unit/effects.js Outdated

Comment thread test/unit/effects.js Outdated

tommyhgunz14 force-pushed the test/callback-only-animation branch from 35b0cad to b0e9578 Compare February 23, 2026 23:09

mgol reviewed Feb 23, 2026

View reviewed changes

Comment thread test/unit/effects.js

tommyhgunz14 force-pushed the test/callback-only-animation branch from b0e9578 to a29f145 Compare February 23, 2026 23:15

mgol approved these changes Mar 4, 2026

View reviewed changes

mgol added Tests Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Mar 4, 2026

mgol requested changes Mar 4, 2026

View reviewed changes

mgol removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Mar 4, 2026

mgol requested changes Mar 26, 2026

View reviewed changes

Comment thread test/unit/effects.js

mgol added Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. and removed Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. Needs review labels Mar 31, 2026

Tests: Skip callback-only animation tests in selector-native build

66284df

The tests use :hidden/:visible pseudo-selectors which require jQuery's custom selector module. Gate them with QUnit.jQuerySelectors to skip when running the selector-native build variant. Ref jquerygh-1738

tommyhgunz14 requested a review from mgol April 1, 2026 21:45

mgol requested changes Apr 1, 2026

View reviewed changes

Comment thread test/unit/effects.js Outdated

tommyhgunz14 requested a review from mgol April 2, 2026 08:38

mgol requested changes Apr 2, 2026

View reviewed changes

Comment thread test/unit/effects.js Outdated

tommyhgunz14 requested a review from mgol April 2, 2026 14:41

mgol approved these changes Apr 2, 2026

View reviewed changes

mgol added Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Apr 2, 2026

mgol requested a review from timmywil April 2, 2026 22:15

timmywil approved these changes Apr 13, 2026

View reviewed changes

timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Apr 13, 2026

mgol merged commit ac83749 into jquery:main Apr 15, 2026
16 checks passed

mgol removed the Needs review label Apr 15, 2026

mgol added this to the 3.8.0 milestone Apr 15, 2026

Conversation

tommyhgunz14 commented Feb 18, 2026

Summary

Problem

Changes

Uh oh!

linux-foundation-easycla Bot commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgol commented Feb 18, 2026

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mgol commented Feb 23, 2026

Uh oh!

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

tommyhgunz14 commented Mar 22, 2026

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgol commented Mar 31, 2026

Uh oh!

tommyhgunz14 commented Apr 1, 2026

Uh oh!

mgol commented Apr 1, 2026

Uh oh!

tommyhgunz14 commented Apr 1, 2026

Uh oh!

tommyhgunz14 commented Apr 1, 2026

Uh oh!

Uh oh!

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

linux-foundation-easycla Bot commented Feb 18, 2026 •

edited

Loading