Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

added optional timeout to phantomjs runner #415

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

milimetric commented Feb 19, 2013

If an async test has a problem and hangs, the phantomjs qunit runner would hang as well. I added an optional timeout as the third parameter and updated the usage text in the readme. I've tested the change with both working and broken async tests.

Owner

JamesMGreene commented Feb 19, 2013

Just to clarify: You are referring to an asyncTest that doesn't call stop() enough times, right?

Contributor

milimetric commented Feb 19, 2013

Correct, that is the example I ran into.
On Feb 19, 2013 12:49 PM, "James M. Greene" notifications@github.com
wrote:

Just to clarify: You are referring to an asyncTest that doesn't call
start() enough times, right?


Reply to this email directly or view it on GitHubhttps://github.com/jquery/qunit/pull/415#issuecomment-13786640.

Owner

JamesMGreene commented Feb 19, 2013

OK, thanks for clarifying. Changes looks good to me. 👍

@ghost ghost assigned JamesMGreene Mar 9, 2013

Owner

JamesMGreene commented Mar 19, 2013

@milimetric: You need to sign the CLA before we can work on merging this PR. Please add a comment here when you've done so. Thanks!

@JamesMGreene JamesMGreene reopened this Mar 19, 2013

Owner

JamesMGreene commented Mar 19, 2013

I see you've signed now, @milimetric. Thanks!

Owner

JamesMGreene commented Mar 19, 2013

Landed, thanks!

Owner

Krinkle commented Mar 19, 2013

Has this proven to be an actual problem? Or just in theory?

Afaik QUnit has a built-in timeout handler that should be more than enough. It is implemented on the test level (not the global level) so we may want to keep this global timeout as well (at the phantomjs level here), but I think in most cases you'll want to keep the global timeout in phantomjs disabled and instead use the one in QUnit so you don't end up with incomplete output when it is aborted (depending on the formatter and whether the formatter lives in the browser or in node, you could end up with invalid json/xml or whatever).

The QUnit.config.testTimeout setting is used by the QUnit.asyncTest and QUnit.stop methods to automatically mark-as-failure and continue if a test takes too long (or doesn't call QUnit.start).

It has the advantage of letting everything else still run (instead of aborting everything). That way you have a more complete list of failures instead of just the first one (makes for more efficient working when fixing them by being able to fix more than one at once instead of having to re-run it after each one to find out the next possible failure).

Another advantage is that the per-test timeout scales better as you don't have to update it periodically as your test suite grows (fast tests but more of them).

As said, the phantomjs timeout can still be useful so we should definitely keep it. But I just wanted to make sure you're also aware of QUnit.config.testTimeout, so you can choose to instead use that (or use both).

Owner

JamesMGreene commented Mar 19, 2013

@Krinkle: As you said, I think this is still valid separately. However, perhaps we need to start documenting more about the QUnit.config options that we indeed to be used by consumers in our docs on api.qunitjs.com/QUnit.config/.

Contributor

milimetric commented Mar 19, 2013

I didn't know about QUnit.config.testTimeout, and I agree with Timo's reasons for
why it's preferable. I also like the phantom one as a fail-safe. As far
as documentation, I agree with James. I'd suggest two small improvements:

Owner

Krinkle commented Mar 19, 2013

Yep, the doc site is still fairly new. There's a lot of things not documented there yet. This is one of them. I've filed jquery/api.qunitjs.com#21.

Owner

jzaefferer commented Mar 19, 2013

These properties were there, we have an infrastructure issue that prevents them from being rendered: jquery/grunt-jquery-content#31 - doesn't make a difference for the user though.

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