Skip to content

Enhancement: @typescript-eslint/no-floating-promises should Ignore node 18's built-in test function #5231

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

Closed
4 tasks done
mgenware opened this issue Jun 23, 2022 · 6 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@mgenware
Copy link

mgenware commented Jun 23, 2022

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-floating-promises/

Description

Example code to run an async test from node docs

import test from 'node:test';

test('asynchronous passing test', async (t) => {
  // This test passes because the Promise returned by the async
  // function is not rejected.
  assert.strictEqual(1, 1);
});

It triggers @typescript-eslint/no-floating-promises so I have to add ignore comments. It would be great if we can ignore the test function from node:test (node 18+).

Fail

// See description.

Pass

// See description.

Additional Info

No response

@mgenware mgenware added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 23, 2022
@bradzacher
Copy link
Member

From the docs

test() returns a Promise that resolves once the test completes. The return value can usually be discarded for top level tests. However, the return value from subtests should be used to prevent the parent test from finishing first and cancelling the subtest

So there are cases you can ignore the promise and cases you can't.

I think this is the perfect candidate for the void operator to mark the promise as explicitly floating:
https://typescript-eslint.io/rules/no-floating-promises/#ignorevoid

We don't want to add specific handling for the semantics of libraries or frameworks. This is just another test framework, even if it's built into node.
Given that there is also a viable workaround already built into the rule - I don't think this is a good thing to build into the rule.

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2022
@bradzacher bradzacher added wontfix This will not be worked on and removed triage Waiting for team members to take a look labels Jun 23, 2022
@JoshuaKGoldberg
Copy link
Member

Hmm, I'm not sure that's the optimal solution @bradzacher. The user story of "if you write native Node tests in linted TypeScript you have to use this operator you've never used this way before" is not ideal. At the very least it'll be confusing for users.

What if we added an option named something like ignoreTopLevel to exclude cases like this? The docs could then recommend that *.test.* files receive an override in the ESLint config.

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for team members to take a look and removed wontfix This will not be worked on labels Jun 23, 2022
@bradzacher
Copy link
Member

What if we added an option named something like ignoreTopLevel to exclude cases like this?

I don't think that this is a good idea because there are many cases where you want to catch top-level floating promises.

One example that comes to mind is the async entrypoint pattern, common case for CLIs:

async function main() {}

main();
// explicitly need handling here so you don't
// have floating promise exceptions leak
// from your entrypoint

Options live forever and people are lazy - so people will just turn on an option instead of using overrides - which creates safety holes! Idk if a safety hole in your application is worth test DevX?

We could change our recommended set to add overrides - though we don't know what a user's test files look like so that's hard (do they use the .spec or .test suffix? Are they suffixless but under __tests__ or tests?). Also overrides are a pain for users to manage as they can't override the override easily.


I hate to say it as I hate these arguments - but there is a slippery slope to consider too.

We don't special case any other things for NodeJS (that I can think of). So if we are special casing this NodeJS api are we going to special case more in future across other rules?

Also what makes the node test framework anything different to one of the much more widely used test frameworks like jest, mocha, chai, etc?
For example the unbound-method rule will "false positive" on jest's expect function.
We asked the jest team to publish their own version of the rule handling their framework's semantics (https://github.com/jest-community/eslint-plugin-jest/blob/HEAD/docs/rules/unbound-method.md).

If we're special casing the new and barely used NodeJS testing framework, should we go back and special case the existing and widely used Jest?


Sorry a bit of a rant - just trying to tease out why this case should be any different to the past stance of "we don't encode framework semantics".

@armano2
Copy link
Collaborator

armano2 commented Jul 20, 2022

i have to agree with brad on this one for now,

adding new option to rule is possible but for now we should wait and see how node:test evolves
node testing framework is currently in experimental stage, and we do not know how and if this behavior is going to stay.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Sep 26, 2022

Coming back to this: it also occurs to me that if node:test sticks with this Promise approach, other frameworks are likely to do the same. And if it(...), test.only(...), etc. APIs also create Promises, an option like ignoreTopLevel wouldn't be enough.

#5271 is a better solution for this & similar issues.

In the meantime, you can either use // eslint-disable-next-line comments or disable the rule for *.test.ts files (or however your project sets up tests).

Long term, if folks want to standardize rule options specific to a framework, that should happen in plugins specific to that framework. In this case, eslint-plugin-node would be the right place to request ignoring Node APIs by default (once #5271 is in).

Thanks all! 🤝

@JoshuaKGoldberg JoshuaKGoldberg added wontfix This will not be worked on and removed triage Waiting for team members to take a look labels Sep 26, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022
@JoshuaKGoldberg
Copy link
Member

Aside: I just filed nodejs/node#51292 on the Node.js issue tracker to start a discussion on whether Node.js should make "floating" Promises this way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants