Skip to content
This repository

Issue #432 - Using a setTimeout stub can stop test suite from continuing #433

Closed
wants to merge 6 commits into from

4 participants

David Vollbracht Jörn Zaefferer Timo Tijhof James M. Greene
David Vollbracht
qxjit commented

If you replace the global setTimeout function with a stub (like sinon's useFakeTimers), it can permanently stop the test suite.

When QUnit stops processing the queue to let the browser update, it calls the global setTimeout to schedule the next block of queue processing. If you are unlucky the test currently running may have stubbed setTimeout and its teardown step could still be on queue waiting to run. In this case, QUnit will end up using the stubbed setTimeout instead of the real one, and the entire test suite will stop running.

This change fixes the problem by capturing window.setTimeout when QUnit is loaded and using the captured version rather than referring to window.setTimeout for each call.

Jörn Zaefferer
Owner

Thanks for the contribution! Could you please sign our CLA? http://contribute.jquery.org/CLA/

David Vollbracht
qxjit commented

My pleasure. I have signed the CLA.

Jörn Zaefferer
Owner

Finally got a chance again to look at this. The test doesn't pass in Firefox. Could you take a look at that, and rebase the PR against master? Thanks.

test/test.js
((19 lines not shown))
  758 + ' setup: function() {' +
  759 + ' this.setTimeout = window.setTimeout;' +
  760 + ' window.setTimeout = function() {};' +
  761 + ' },' +
  762 + ' teardown: function() {' +
  763 + ' window.setTimeout = this.setTimeout;' +
  764 + ' }' +
  765 + ' });' +
  766 + ' test("just a test", function() { ok(true); });' +
  767 + ' test("just a test", function() { ok(true); });' +
  768 + ' </script>' +
  769 + ' </body>' +
  770 + ' </html>';
  771 +
  772 + var frame = document.createElement('iframe');
  773 + var supportsSrcDoc = !!('srcdoc' in frame);
1
Jörn Zaefferer Owner
jzaefferer added a note

The in test already produces a boolean, no need for the explicit cast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
David Vollbracht
qxjit commented

Sorry - was sick last week and didn't get to it. I'm happy to rebase and fix whatever is happening in firefox this week.

David Vollbracht
qxjit commented

I have fixed the firefox issue (innerText, DOH!) and rebased on latest master. Let me know if there's anything else you'd like me to do.

Jörn Zaefferer
Owner

Thanks, landed!

Timo Tijhof Krinkle referenced this pull request from a commit
Timo Tijhof Krinkle Revert "Fixes #432 - Using a setTimeout stub can stop test suite from…
… continuing - closes gh-433"

This reverts commit cf41077.
b56ea44
Timo Tijhof Krinkle reopened this
Timo Tijhof
Collaborator
Krinkle commented

This fails in IE8, IE7 and IE6:

Died on test #1 undefined: Object doesn't support this property or method 

http://swarm.jquery.org/job/553

James M. Greene
Collaborator

Probably the Date.now() calls. They should be changed to +new Date() or (new Date()).getTime().

Timo Tijhof
Collaborator
Krinkle commented

Also making use of this oppertunity to point out:

  • Code does not conform to the style guide
  • Does it need the whole iframe thing? Shouldn't it be possible as a regular module with setup/teardown? I suspect the QUnit.config setting is why it is in a separate test suite, however in that case please add a regular test suite in an html file instead of re-inventing QUnit Composite locally.
David Vollbracht
qxjit commented

If i create a separate html file for its own test suite, the failure mode of that test suite will be to hang forever without actually reporting failure. Is that ok?

David Vollbracht
qxjit commented

I don't have access to IE at the moment, but I will make the date change. Is there a way for me to run the test swarm from my local copy?

David Vollbracht
qxjit commented

I'm happy to digest the style guide and ensure the code conforms to it. I'll starting looking at it tomorrow morning.

Timo Tijhof
Collaborator
Krinkle commented

@qxjit Not TestSwarm, no. But the logic TestSwarm and Jenkins perform is only 1% of the magic. Pretty much everything is the repository itself.

You can open test/index.html of this repository in your browser and run it. Simple as that.


  • Checkout jquery/qunit.git (be sure to include submodules and run npm install).
  • Open http://localhost/jquery/qunit/test (or wherever you put it within your web server doc root) in Internet Explorer. If you (like most of us) don't run Windows and don't have Internet Explorer locally, you can open the URL in either using your own VM or a cloud service that provides a VM. I'd recommend the latter.
    A few cloud services that feature real Internet Explorer access (e.g. not IETester or MultipleIE):
    • SauceLabs (has free/opensource option)
    • BrowserStack

Both of these have easy-to-use and built-in support to set up a tunnel in your browser to forward your localhost, and they have in-browser (using HTML/javascript) viewport of the remote VM that is instantly spawned for you.

After initial set up it takes less than 10 seconds for me to point my web browser (e.g. Chrome) to BrowserStack, paste my local url, pick OS/browser/version, start.

David Vollbracht
qxjit commented

I've moved the tests to their suite and replaced spaces with tabs. I think that brings the code into conformance with the style guide. The only thing I'm not sure about is how to integrate the new suite into test swarm. I assume that's something that needs to done.

David Vollbracht
qxjit commented

I believe this now conforms to the style guide, and I've separated the tests to their own suite. Does anything need to be done to make it work with the TestSwarm build?

Jörn Zaefferer jzaefferer closed this pull request from a commit
David Vollbracht qxjit Use a local setTimeout reference, add separate unit test suite for th…
…at. Fixes #432 - Using a setTimeout stub can stop test suite from continuing. Closes gh-433
4e62b4a
Timo Tijhof Krinkle commented on the diff
test/setTimeout.html
... ... @@ -0,0 +1,15 @@
  1 +<!DOCTYPE html>
  2 +<html>
  3 + <head>
  4 + <meta charset="UTF-8">
  5 + <title>QUnit Fake setTimeout Test Suite</title>
  6 + <link rel="stylesheet" href="../qunit/qunit.css">
  7 + <script src="test.js"></script>
  8 + <script src="deepEqual.js"></script>
  9 + </head>
  10 + <body>
  11 + <div id="qunit"></div>
  12 + <script src="../qunit/qunit.js"></script>
  13 + <script src="setTimeout.js"></script>
2
Timo Tijhof Collaborator
Krinkle added a note

This also needs swarminject.js.

Timo Tijhof Collaborator
Krinkle added a note

Fixed in 65d27d2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jörn Zaefferer
Owner

Thanks, landed! I squashed, removed the extra scripts (test.js, deepEqual.js), added swarminject (in 65d27d2) and a bit of spacing in the JS file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
3  Gruntfile.js
@@ -11,7 +11,8 @@ grunt.initConfig({
11 11 qunit: [
12 12 "test/index.html",
13 13 "test/async.html",
14   - "test/logs.html"
  14 + "test/logs.html",
  15 + "test/setTimeout.html"
15 16 ]
16 17 },
17 18 jshint: {
7 qunit/qunit.js
@@ -20,6 +20,7 @@ var QUnit,
20 20 hasOwn = Object.prototype.hasOwnProperty,
21 21 // Keep a local reference to Date (GH-283)
22 22 Date = window.Date,
  23 + setTimeout = window.setTimeout,
23 24 defined = {
24 25 setTimeout: typeof window.setTimeout !== "undefined",
25 26 sessionStorage: (function() {
@@ -466,7 +467,7 @@ QUnit = {
466 467 }
467 468 // A slight delay, to avoid any current callbacks
468 469 if ( defined.setTimeout ) {
469   - window.setTimeout(function() {
  470 + setTimeout(function() {
470 471 if ( config.semaphore > 0 ) {
471 472 return;
472 473 }
@@ -489,7 +490,7 @@ QUnit = {
489 490
490 491 if ( config.testTimeout && defined.setTimeout ) {
491 492 clearTimeout( config.timeout );
492   - config.timeout = window.setTimeout(function() {
  493 + config.timeout = setTimeout(function() {
493 494 QUnit.ok( false, "Test timed out" );
494 495 config.semaphore = 1;
495 496 QUnit.start();
@@ -1445,7 +1446,7 @@ function process( last ) {
1445 1446 if ( !defined.setTimeout || config.updateRate <= 0 || ( ( new Date().getTime() - start ) < config.updateRate ) ) {
1446 1447 config.queue.shift()();
1447 1448 } else {
1448   - window.setTimeout( next, 13 );
  1449 + setTimeout( next, 13 );
1449 1450 break;
1450 1451 }
1451 1452 }
15 test/setTimeout.html
... ... @@ -0,0 +1,15 @@
  1 +<!DOCTYPE html>
  2 +<html>
  3 + <head>
  4 + <meta charset="UTF-8">
  5 + <title>QUnit Fake setTimeout Test Suite</title>
  6 + <link rel="stylesheet" href="../qunit/qunit.css">
  7 + <script src="test.js"></script>
  8 + <script src="deepEqual.js"></script>
  9 + </head>
  10 + <body>
  11 + <div id="qunit"></div>
  12 + <script src="../qunit/qunit.js"></script>
  13 + <script src="setTimeout.js"></script>
  14 + </body>
  15 +</html>
15 test/setTimeout.js
... ... @@ -0,0 +1,15 @@
  1 +QUnit.config.updateRate = 1;
  2 +
  3 +module("Module that mucks with time", {
  4 + setup: function() {
  5 + this.setTimeout = window.setTimeout;
  6 + window.setTimeout = function() {};
  7 + },
  8 +
  9 + teardown: function() {
  10 + window.setTimeout = this.setTimeout;
  11 + }
  12 +});
  13 +
  14 +test("just a test", function() { ok(true); });
  15 +test("just a test", function() { ok(true); });

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.