Tests: Refactor iframe tests for async ready event #3049
Conversation
Ref jquerygh-3040 Not really happy with this solution for the long term, it seems like should be having the iframe code signal when it's done. The old code would have broken for any use of Deferred or setTimeout in the iframe, and it's likely this will break too.
|
By analyzing the blame information on this pull request, we identified @timmywil, @markelog and @gibson042 to be potential reviewers |
|
This seems pretty fragile... |
|
Completely agree. The better solution is to have the iframe call back to let the parent know it's done. That requires some (simple I think) changes to all the tests run via We can either merge this to make the problem go away now or someone can do the right thing when they get a chance. (Or revert the async ready code I guess.) |
|
There are only four uses of
Especially considering that ready is an excludable module, I'd like to do things the right way and drop all four, replacing |
|
Okay, since @gibson042 did the searching and I have about 90 minutes now, imma try to get this fixed the right way. Hold my coat I'm going in! |
|
@dmethvin I assume someone got your coat. Are you taking anything else off? |
|
WHY ARE THE FIRST TWO ARGS REVERSED FOR THESE GUYS???! |
|
WHY DOES testIframe add the |
|
What a mess! There were a bunch of other |
|
/play rollout |
|
Huh, I don't know how I missed those. At any rate, thanks @dmethvin. You are a gentleman and a scholar. |
README.md
Outdated
| ```js | ||
| callback( jQueryFromIFrame, iFrameWindow, iFrameDocument ); | ||
| testIframeWithCallback( testName, fileName, | ||
| function callback( arg1, arg2, ... assert ) { |
There was a problem hiding this comment.
This syntax looks weird to me. Did you mean:
function callback( arg1, arg2, ..., assert ) {
...
} );?
|
That's great! 👏 |
|
I added another commit to clean up a bunch of things. Now each iframe page includes a file that defines |
dmethvin
changed the title
| @@ -0,0 +1,9 @@ | |||
| // Just used in iframed html tests which jshint doesn't see | |||
| // jshint unused: false | |||
There was a problem hiding this comment.
window.startIframeTest = function() {} should help with jshint?
7 participants
Ref gh-3040
Not really happy with this solution for the long term, it seems like should beSee below.having the iframe code signal when it's done. The old code would have broken for
any use of Deferred or setTimeout in the iframe, and it's likely this will break
too.