- Notifications
You must be signed in to change notification settings - Fork20.6k
Tests: Fix flakiness in the "jQuery.ajax() - JSONP - Same Domain" test#4687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The "jQuery.ajax() - JSONP - Same Domain" test is firing a request witha duplicate "callback" parameter, something like (simplified):```mock.php?action=jsonp&callback=jQuery_1&callback=jQuery_2```There was a difference in how the PHP & Node.js implementations of the jsonpaction in the mock server handled situations like that. The PHP implementationwas using the latest parameter while the Node.js one was turning it into anarray but the code didn't handle this situation. Because of how JavaScriptstringifies arrays, while the PHP implementation injected the following code:```jsjQuery_2(payload)```the Node.js one was injecting the following one:```jsjQuery_1,jQuery_2(payload)```This is a comma expression in JavaScript; it so turned out that in the majorityof cases both callbacks were identical so it was more like:```jsjQuery_1,jQuery_1(payload)```which evaluates to `jQuery_1(payload)` when `jQuery_1` is defined, making thetest go as expected. In many cases, though, especially on Travis, the callbackswere different, triggering an `Uncaught ReferenceError` error & requiringfrequent manual re-runs of Travis builds.This commit fixes the logic in the mock Node.js server, adding special handlingfor arrays.
An example failing build (with some debug logging details):https://travis-ci.org/github/jquery/jquery/jobs/678811015 You can see the error in the logs:
|
It took me a few months of fighting with this issue to finally take the time to find the culprit. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Your persistence paid off!
The "jQuery.ajax() - JSONP - Same Domain" test is firing a request witha duplicate "callback" parameter, something like (simplified):```mock.php?action=jsonp&callback=jQuery_1&callback=jQuery_2```There was a difference in how the PHP & Node.js implementations of the jsonpaction in the mock server handled situations like that. The PHP implementationwas using the latest parameter while the Node.js one was turning it into anarray but the code didn't handle this situation. Because of how JavaScriptstringifies arrays, while the PHP implementation injected the following code:```jsjQuery_2(payload)```the Node.js one was injecting the following one:```jsjQuery_1,jQuery_2(payload)```This is a comma expression in JavaScript; it so turned out that in the majorityof cases both callbacks were identical so it was more like:```jsjQuery_1,jQuery_1(payload)```which evaluates to `jQuery_1(payload)` when `jQuery_1` is defined, making thetest go as expected. In many cases, though, especially on Travis, the callbackswere different, triggering an `Uncaught ReferenceError` error & requiringfrequent manual re-runs of Travis builds.This commit fixes the logic in the mock Node.js server, adding special handlingfor arrays.Closesgh-4687(cherry picked from commit7b0864d)
Summary
The "jQuery.ajax() - JSONP - Same Domain" test is firing a request with
a duplicate "callback" parameter, something like (simplified):
There was a difference in how the PHP & Node.js implementations of the jsonp
action in the mock server handled situations like that. The PHP implementation
was using the latest parameter while the Node.js one was turning it into an
array but the code didn't handle this situation. Because of how JavaScript
stringifies arrays, while the PHP implementation injected the following code:
the Node.js one was injecting the following one:
This is a comma expression in JavaScript; it so turned out that in the majority
of cases both callbacks were identical so it was more like:
which evaluates to
jQuery_1(payload)
whenjQuery_1
is defined, making thetest go as expected. In many cases, though, especially on Travis, the callbacks
were different, triggering an
Uncaught ReferenceError
error & requiringfrequent manual re-runs of Travis builds.
This commit fixes the logic in the mock Node.js server, adding special handling
for arrays.
Checklist
If needed, a docs issue/PR was created athttps://github.com/jquery/api.jquery.com