Skip to content

print pending tests on interrupt #2022

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

Merged
merged 3 commits into from
Jan 27, 2019
Merged

Conversation

dflupu
Copy link
Contributor

@dflupu dflupu commented Jan 19, 2019

Fixes #583

@dflupu dflupu force-pushed the print-on-interrupt branch 3 times, most recently from 20a97d7 to 8283ff8 Compare January 19, 2019 19:55
@dflupu
Copy link
Contributor Author

dflupu commented Jan 19, 2019

Can't get that test working when ran during npm test. tap --no-cov test/integration/assorted.js works fine. I assume the event loop is being blocked by something else.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Thanks for another stellar PR @dflupu! Some refactoring may have unintended consequences though, see below.

Can't get that test working when ran during npm test. tap --no-cov test/integration/assorted.js works fine. I assume the event loop is being blocked by something else.

I'll have a look (at some point).

t.end();
});
});

test('interrupt', t => {
const proc = execCli(['long-running.js', '-T', '5s'], (err, stdout) => {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove the -T argument to make it clearer this only deals with SIGINT.

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've removed this test altogether since it didn't work during npm test. For reference, here it was:

test('interrupt', t => {
	const proc = execCli(['long-running.js'], (_, stdout) => {
		t.match(stdout, /SIGINT/);
		t.end();
	});

	setTimeout(() => {
		proc.kill('SIGINT');
	}, 2000);
});

@dflupu dflupu force-pushed the print-on-interrupt branch from 8283ff8 to 4e87135 Compare January 22, 2019 23:43
@dflupu dflupu force-pushed the print-on-interrupt branch from 4e87135 to befde13 Compare January 23, 2019 00:05
@dflupu
Copy link
Contributor Author

dflupu commented Jan 23, 2019

@novemberborn Can you take a look at the replies above? I don't know if you get notifications for them so I figured I'd poke you in a separate comment. :)

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Odd, I'm not sure why the SIGINT test fails in CI. It passes locally. I've added it back but commented out. Hopefully it'll start to work at some point…

@novemberborn novemberborn merged commit 2b60556 into avajs:master Jan 27, 2019
@sindresorhus
Copy link
Member

@dflupu You need to submit the PR URL to IssueHunt to claim the bounty: https://issuehunt.io/repos/26820798/issues/583

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