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 spurious "test exited without ending" #374

Merged
merged 2 commits into from
Jun 26, 2017
Merged

Fix spurious "test exited without ending" #374

merged 2 commits into from
Jun 26, 2017

Conversation

feross
Copy link
Contributor

@feross feross commented Jun 24, 2017

Fixes: #223

This PR ensures that all test files are required on the first tick.

To make tape happy, all tests need to be queued on the first tick. Tape assumes that tests are done when there's nothing in the queue.

Right now, the tape binary is asynchronously resolving the globs and queuing up the tests as those callbacks are called. This can lead to spurious "test exited without ending" errors.

@Raynos identified this line as the issue a while back #223 (comment) Let's finally fix this :)

@kgryte
Copy link

kgryte commented Jun 24, 2017

If the glob fails, the forEach line will throw, correct? Is this desired behavior?

bin/tape Outdated
});
var files;
try {
files = glob.sync(arg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

When in the async version would an error be generated by glob, compared to the sync version?

Separately, I appreciate that the current PR mirrors the original version's error handling precisely; but it might be better to let glob.sync's error throw, rather than having an undefined.forEach error. I'd consider that change semver-patch, since it's just making the error message more helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When in the async version would an error be generated by glob, compared to the sync version?

I expect would be generated in the same situations. The docs are unclear about which conditions generate errors. In the case that a glob matches no files, an empty array of files is returned (no error).

I agree with you about undefined.forEach being unhelpful. I will update the PR so that glob.sync throws. An alternative would be to suppress/ignore any errors (as the original code was doing).

Copy link
Collaborator

Choose a reason for hiding this comment

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

To my reading, the original code would still throw the undefined.forEach error, it'd just be thrown from the async callback.

@ljharb ljharb added the bug label Jun 24, 2017
@kgryte
Copy link

kgryte commented Jun 24, 2017

For async logic which achieves the same goal (but in a different context), see here.

@feross
Copy link
Contributor Author

feross commented Jun 26, 2017

The PR is updated now to address @ljharb's feedback.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb merged commit 51597e2 into tape-testing:master Jun 26, 2017
@ljharb
Copy link
Collaborator

ljharb commented Jun 26, 2017

I'll cut a release of this later today.

@feross
Copy link
Contributor Author

feross commented Jun 27, 2017

@ljharb Nice, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"test exited without ending"
3 participants