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

test: proposal common.expectsError nextTick option #29749

Closed
ronag opened this issue Sep 28, 2019 · 3 comments
Closed

test: proposal common.expectsError nextTick option #29749

ronag opened this issue Sep 28, 2019 · 3 comments

Comments

@ronag
Copy link
Member

ronag commented Sep 28, 2019

I would like to propose a new option for common.expectsError which will indicate whether the error can be invoked in the same tick or not, e.g.

I would like to be able to replace tests like this:

let ticked = false;
stream.write(null, common.mustCall((err) => {
  assert.strictEqual(ticked, true);
  common.expectsError(opts)(err);
}));
ticked = true;

with

stream.write(null, common.expectsError({
 ...opts,
 nextTick: true
});

Would this make sense? It's quite often that one wants to test for callback errors and that they should be invoked asynchronously.

I think this would also help us actually making sure that callbacks are invoked asynchronously.

NOTE: Can I use expectsError to test for no error?

@ronag ronag changed the title test: proposal common.expectsError tick test: proposal common.expectsError nextTick option Sep 28, 2019
@Trott
Copy link
Member

Trott commented Sep 28, 2019

I (probably) wouldn't block it, but I think the first example is clear. I think the proposed API results in code where it is not immediately obvious what is going on. So someone will have to go and read the documentation, which increases friction.

If anything, in the first example, I might avoid common.expectsError() and use one or more calls to assert.strictEqual() instead. The test code will be longer, but it will be clearer. No one will have to learn our common.expectsError() function to understand the test.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 28, 2019

I share @Trott's opinion on this issue.

I'd actually like to see common.expectsError() go away completely, but that's not the topic of this issue 😄

@ronag
Copy link
Member Author

ronag commented Sep 28, 2019

Makes sense.

@ronag ronag closed this as completed Sep 28, 2019
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

No branches or pull requests

3 participants