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

Fix Windows onExit failure. #169

Closed
wants to merge 2 commits into from

Conversation

jamestalmage
Copy link
Contributor

SIGHUP seems to be the only kill signal supported by Windows:
https://nodejs.org/api/process.html#process_process_kill_pid_signal

This is a stab in the dark while I wade through setting up Node on Windows.

@jamestalmage
Copy link
Contributor Author

😢 If only it were that easy.

@sindresorhus
Copy link
Member

You're dealing with computers. It's never easy :p

@jamestalmage jamestalmage reopened this Nov 8, 2015

childProcess.execFile('../cli.js', args, {cwd: __dirname}, cb);
childProcess.execFile(node, args, {cwd: __dirname}, cb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use childProcess.execFile(process.execPath, ['../cli.js'].concat(args), {cwd: __dirname}, cb); instead of which.

@jamestalmage
Copy link
Contributor Author

Finally!
@sindresorhus - I'm gonna let the latest push go green and then squash

@@ -158,7 +158,10 @@ function exit(results) {
// correctly flush the output when multiple test files
process.stdout.write('');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have actually fixed this line too.

Do we have a test that covers this?

Everything passed on OSX / Node 4.1 without this and without my change (in other words, unless the problem this addresses is specific to an OS / Node version we do NOT have test coverage for it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdemedes, this line is from #47

Can you remember what you were addressing there? Tests pass without it. (even before my changes).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.stdout.write(''); was there before me, @sindresorhus should have a reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Sorry - blame attributed it to you, and I didn't scan down the #47 far enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added it as a quick hack because the 1 test passed message wouldn't output without it when running multiple tests and I couldn't reproduce it in a test. If it works fine now you can just remove it, things have changed so much since then.

@jamestalmage jamestalmage changed the title Fix windows onExit failure. Fix Windows onExit failure. Nov 9, 2015

process.on('message', function (message) {
if (message['ava-kill-command']) {
process.exit(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the test process is now receiving messages.

Do we need to monkey patch process.on so tests never see them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unless it interferes with them. @vdemedes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't interfere with AVA test code.

But if someone attaches a process.on('message', handler), and then emits their own message events on process, that would interfere. Though it would be really odd behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't really ever make sense for them to do that though (not unless they fork an additional process).

@jamestalmage
Copy link
Contributor Author

@sindresorhus @vdemedes
I've made the recommended changes. Anything else holding this up? It would be nice if peoples PR's could go green again.

@sindresorhus
Copy link
Member

Landed! Superb work tracking this down @jamestalmage. Node.js 0.10 is getting so annoying to support...

@vadimdemedes
Copy link
Contributor

I just realized something. Why don't we just let forks exit themselves, without interaction with a parent process?

process.send(results);
process.exit();

@jamestalmage
Copy link
Contributor Author

I don't think there is anything stopping us from doing that now.

I did have one thought with regard to #140, and that is to leave processes running as long as possible to give us the best chance of catching extra assertion calls (beyond plan), and repetitive calls to t.end()

In that case it would be up to the parent to close.

But if we think that is a bad idea, then no - there is probably no reason to do it the way we are.

@jamestalmage jamestalmage deleted the fix-windows-failure branch November 21, 2015 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants