added optional timeout to phantomjs runner

[milimetric]

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.

[JamesMGreene]

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

[milimetric]

Correct, that is the example I ran into.
On Feb 19, 2013 12:49 PM, "James M. Greene" [email protected]
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//pull/415#issuecomment-13786640.

[JamesMGreene]

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

[JamesMGreene]

@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]

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

[JamesMGreene]

Landed, thanks!

[Krinkle]

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).

[JamesMGreene]

@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/.

[milimetric]

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:

• These couple of search queries should return some info:
  http://api.qunitjs.com/?s=QUnit.config.testTimeout and
  http://api.qunitjs.com/?s=config+testTimeout
• This page could have a bullet point about each potentially useful
  internal config: http://api.qunitjs.com/QUnit.config/

[Krinkle]

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 qunitjs/api.qunitjs.com#21.

[jzaefferer]

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.