Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async causes tests to abruptly stop #1132

Open
getify opened this issue Mar 30, 2017 · 16 comments
Open

async causes tests to abruptly stop #1132

getify opened this issue Mar 30, 2017 · 16 comments

Comments

@getify
Copy link

@getify getify commented Mar 30, 2017

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?

@getify
Copy link
Author

@getify getify commented Mar 30, 2017

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

@platinumazure
Copy link
Contributor

@platinumazure platinumazure commented Mar 30, 2017

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?

@getify
Copy link
Author

@getify getify commented Mar 30, 2017

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.

@leobalter
Copy link
Member

@leobalter leobalter commented Mar 30, 2017

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

@platinumazure
Copy link
Contributor

@platinumazure platinumazure commented Mar 30, 2017

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

@leobalter
Copy link
Member

@leobalter leobalter commented Mar 30, 2017

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.

@trentmwillis
Copy link
Member

@trentmwillis trentmwillis commented Apr 6, 2017

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

@trentmwillis trentmwillis self-assigned this Apr 6, 2017
@trentmwillis
Copy link
Member

@trentmwillis trentmwillis commented Apr 9, 2017

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.
@puppetmaster3
Copy link

@puppetmaster3 puppetmaster3 commented Apr 18, 2017

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

@getify
Copy link
Author

@getify getify commented Jul 15, 2017

Any update on this?

@rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Dec 19, 2017

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 }
@rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Dec 19, 2017

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

@getify
Copy link
Author

@getify getify commented Dec 19, 2017

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

@rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Dec 19, 2017

@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)
@Krinkle
Copy link
Member

@Krinkle Krinkle commented Mar 6, 2018

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
@Krinkle
Copy link
Member

@Krinkle Krinkle commented Oct 4, 2019

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

Regarding the handling of abrupt exists, 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.

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

Successfully merging a pull request may close this issue.

None yet
7 participants