qunit

qunit copied to clipboard

I'm not sure if this behavior is by-design, or if there's just no way for QUnit to fix it, but this situation just bit me and it was very confusing.

Here's my minimal test case:

"use strict";

var QUnit = require("qunitjs");

QUnit.done(function done(results) {
	console.log("done:",results);
});

QUnit.test("temp",function(assert){
	var done = assert.async();
	assert.expect(1);

	setTimeout(function(){
		console.log("timer!");
	},1000);
});

QUnit.test( "temp2", function(assert){
	assert.expect(1);
	assert.ok(true,"temp2");
});

QUnit.start();

And when I run this:

$] node tmp.js
timer!
$]

In other words, the first test case fires the timer, but since it never runs the done(), it just silently dies. Since node doesn't have any other waiting event handlers, node just exits. The test suite doesn't fire my completion handler to notify me that a test failed to meet its assertion, and it doesn't try to run the second test at all. It just stops.

And even worse, the test suite exits with a zero exit code, meaning my other CLI tools consider this a passing test suite run. :(

This seems like broken behavior to me. Shouldn't there be some sort of timeout in the background for async tests, or something like that?

Or maybe qunit could register a process.on("exit", ..) handler, that if the test suite hasn't completed by the time the process tries to finish, the handler forces a non-zero exit code to signal failure to the CLI, and emits some error about an abnormally terminated test suite?

Should mention: I'm using v2.3.0 and node 7.7.4.

It definitely feels like a bug to me. On the browser, the tests would just hang indefinitely and the run would not be considered a passing run.

I like the idea of using process.on("exit") to detect abnormal termination, since we should be able to track if all registered tests have run or not.

Regarding timeouts, I wouldn't want to implement one by default but I would be open to an option for the same. I believe we have per-test timeouts supported in the browser, but I don't know if the CLI supports it yet.

@trentmwillis Any thoughts on this?

Regarding timeouts, I wouldn't want to implement one by default but I would be open to an option for the same.

Similar to assert.expect(..), it would be useful if assert.timeout(..) could be called to provide a timeout delay, after which the test would be considered a failure.

On a related topic, assert.failure() would be useful to proactively mark the test as failed, so that I could do my own timeouts... like setTimeout( assert.failure, 1000 ) for example.

that's right. QUnit is not holding node until the tests reach completion.

@leobalter Looks like the CLI should be handling this via these lines in bin/run.js. But that code is not invoked when the Node API is used via require("qunitjs").

I'm not really sure it should be up to QUnit core to handle this (it should be runtime-agnostic). I think if someone is using the Node API, maybe it should be up to them to listen for the runEnd event themselves... What do you think? (Another option would be to just register a noop function in a Node-specific wrapper around core, but...)

EDIT: On second thought, I think the correct solution is to create a Node wrapper which basically inherits from the core QUnit object. And it should use something like QUnit.on( "runEnd", function() { process.nextTick( process.exit ); }); to hold the event loop until the test is done. We can augment with timeout events later to allow some conditional forced exiting.

I think if someone is using the Node API, maybe it should be up to them to listen for the runEnd event themselves

That's how we handled it in our grunt task.

What do you think? Another option would be to just register a noop function in a Node-specific wrapper around core, but...

Now I wonder if we would break previous implementations by "fixing" this.

I'll be looking into this, planning on doing several fixes/improvements related to assert.async.

So there are a couple parts to this issue:

1. Currently, we have QUnit.config.testTimeout which should rectify the issue here, however it is undefined by default. I think this should be set to some reasonable default (60 seconds maybe?), though this might be considered a breaking change.
2. In the case where QUnit.config.testTimeout is set to undefined (or any falsy value), we should check for early exits of the process. We can do this in our CLI implementation, relatively easily I suspect.
3. Related to (2), I think the CLI should likely default to exiting with a non-zero status code, unless the test suite has finished successfully, this will help us avoid false-positives.
4. There is no easy way to set a per-test timeout. I think we should implement a assert.timeout() method.

+1. I need a good async/promise testing for fetch()

Any update on this?

tl;dr I believe that running tests with qunit on the command line has successfully mitigated the issues reported, however running the script (from the description) via node some-file.js still exhibits poor behavior (e.g. exit code of 0, no failure messaging, etc).

---

Running this script with qunit test-script.js:

// test-script.js
QUnit.done(function done(results) {
	console.log("done:",results);
});

QUnit.test("temp",function(assert){
	var done = assert.async();
	assert.expect(1);

	setTimeout(function(){
		console.log("timer!");
	},1000);
});

QUnit.test( "temp2", function(assert){
	assert.expect(1);
	assert.ok(true,"temp2");
});

Results in the following output:

❯❯❯ bin/qunit test-script.js
TAP version 13
timer!
Error: Process exited before tests finished running
Last test to run (temp) has an async hold. Ensure all assert.async() callbacks are invoked and Promises resolve. You should also set a standard timeout via QUnit.config.testTimeout.
❯❯❯ echo $?
1

I believe the main pain points are called out in the output above:

1. Process exited before tests are completed message
2. Exit code is non-zero
3. A warning is printed with helpful information about what to do

If we follow the warnings guidance and add a value for QUnit.config.testTimeout (e.g. 5000) we get a failed test, and better console output:

TAP version 13
timer!
not ok 1 temp
  ---
  message: "Test took longer than 5000ms; test timed out."
  severity: failed
  actual: null
  expected: undefined
  stack:     at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
  ...
ok 2 temp2
1..2
# pass 1
# skip 0
# todo 0
# fail 1
done: { passed: 1, failed: 1, total: 2, runtime: 5027 }

The remaining question is: do we want to directly support running the tests without the CLI like in the original issue report?

I have about a dozen projects where i run my own node CLI tests as above. Never knew that some special CLI was required.

@getify - The CLI (provided when installing npm i -g qunit) handles a number of ergonomic issues over simply doing require('qunit') (and AFAIK is the only documented way to use QUnit in node?). The issue is that the code used for the main entry point (qunit/qunit.js) is generally shared between browser and node and therefore tries to walk a fine line around including node or browser specific code there.

There are definitely things that can be done to make the direct require('qunit') use case better:

• Emit a warning when assert.async() is invoked but no test timeout is present
• Decide if we can add a default QUnit.config.testTimeout in a SemVer compatible way (unclear to me)

I vaguely expected our recent commits in master to have fixed this but I can still reproduce the issue.

$ node tmp.js
timer!
$ # exit code: 0

Only timer! is logged and zero exit code.

However, recent commits did address the problem for QUnit CLI, and surfaces the issues quite well:

$ qunit tmp.js
TAP version 13
timer!
Error: Process exited before tests finished running
Last test to run (temp) has an async hold. Ensure all assert.async() callbacks are
invoked and Promises resolve.
You should also set a standard timeout via QUnit.config.testTimeout.
1 $ # exit code: 1

Circling back to this, I think we might be able to find a middle ground.

Regarding the handling of abrupt exits, and reporting of unhandled exceptions/promises, that is imho in the realm of reporters and plugins. For example, the HTML reporter is responsible for handling events from window.onerror and turning those into QUnit events.

The example snippet in the issue description does not have a reporter registered (other than a basic done callback). As such, I think it's acceptable for these realm-specific events not to be handled. And as brought up by previous comments, doing so might actually be unexpected in some case and/or break compatibility. Having this happen on require('qunit') would imho be too invasive.

To use the TAP reporter, for example:

const QUnit = require('qunit');
const { TapReporter } = require('js-reporters');

TapReporter.init(QUnit);
QUnit.on('runEnd', (results) => {
	console.log('runEnd:', results);
	if (results.status === 'failed') {
		process.exit(1);
	}
});

However, there's two things I think we can (and should) do:

1. Set a default timeout going forward, e.g. 10s?
2. Expose our Node.js logic in a way that can be easily called from a standalone script (e.g. require('qunit/node').init()).

Doing either of these would solve the issue at hand. Doing both would be great, too.