node:test APIs returning Promises clashes with no-floating-promises lint rule

[JoshuaKGoldberg]

Version

21.5.0

Platform

n/a

Subsystem

node:test

What steps will reproduce the bug?

1. Write code that calls a describe, it, or test function imported from node:test
2. Lint that code with @typescript-eslint/no-floating-promises enabled

I put a standalone repro on https://github.com/JoshuaKGoldberg/repros/tree/node-test-no-floating-promises. From its README.md:

import { it } from "node:test";

it("", () => {});
// Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator.
// eslint(@typescript-eslint/no-floating-promises)

How often does it reproduce? Is there a required condition?

100% of the time. The most common workarounds are to either disable the rule in test files or use the rule's ignoreVoid option.

What is the expected behavior? Why is that the expected behavior?

@typescript-eslint/no-floating-promises is enabled in the plugin:@typescript-eslint/recommended-type-checked config that is recommended to users as part of typed linting. I was expecting that the built-in functions for Node.js wouldn't directly trigger complaints in the built-in presets for typescript-eslint.

What do you see instead?

Lint failures out of the box.

---

As for what should be done: I'm not sure 🤔. If it were just up to me, I'd say switching the functions to have void returns ... or failing that, having their @types/node types changed to return void to indicate the returned values shouldn't be used.

How intentional is it that the functions' returned values are made available to users? I can see an argument that returning a Promise that rejects or resolves is a potentially useful feature for the functions...

Additional information

I wasn't sure whether to file this as an issue or discussion. It feels like an issue to me because common practice for many TypeScript projects has been to enable this rule, or previously TSLint's no-floating-promises. But I'm not a Node.js expert - apologies if I'm off base 🙂. If the rule is wrong to complain for some general-to-JavaScript reason, then I'd be happy to take lead on a general bug in typescript-eslint.

We first received this as a bug report in typescript-eslint/typescript-eslint#5231. We wontfixed that as it's not a good idea for a linter to add framework-specific behaviors. We're instead discussing adding an allow option to @typescript-eslint/no-floating-promises. But, that wouldn't be ideal for users IMO because then everyone would have to add an explicit allow to their ESLint configs or use a shared config/preset.

[aduh95]

/cc @nodejs/test_runner

[ljharb]

I've never seen a test framework where it returns anything but void/undefined; this feels like an oversight.

That said, an additional oversight would be that the promise currently returned from it needs to actually be handled by the test runner.

[bradzacher]

I've personally found it's kinda confusing because there's the top level promises which you shouldn't await and then there's the inner promises you should await. But there's the global functions and the inner functions passes into your test closure - all of the same names.

Also sometimes I wanted to await the promise within a test closure - sometimes I didn't.

I had to try combinations of things to figure out exactly what to do and why certain things didn't work.

[felixfbecker]

Is there anything speaking against just recommending (in examples in the docs) to await these promises, given we have top-level await? Since the test suite functions can be async (which is a useful feature because it means you can read fixtures from files etc) it seems logical to me that test()/it() should be awaited

[mcollina]

I think the no-floating-promises rule is problematic and sincerely limit the capability of frameworks.

The problem stem from the fact that promises could either be a request/response primitive, or a fire-and-forget one. The rule does not allow the maintainer/author of the library to indicate the distinction: there are certain promises that must be awaited, and others that can be completely avoided.

The no-floating—promises rule also apply to Thenables (which are not promise), which caused me various headaches in the past.

You can read the full explanation of all of this at typescript-eslint/typescript-eslint#2640.
What is missing in the rule is to allow maintainers to comment functions and let them bypass the rulez

Given the response in the linked issue, it does not seem there is any kind of will to have this sorted. Therefore, my recommendation is to stop using this rule, it prevents frameworks from using idiomatic and better DX.

Last but not least, I think the ship has sailed from changing this, because:

1. it solves a user need elegantly
2. changing it would be a semver-major change

I’m happy to talk to anybody on the tslint team about this.

[JoshuaKGoldberg]

Thanks for the detailed response @mcollina!

it solves a user need elegantly

Clarifying: what is that user need, and how often is it come up?

changing it would be a semver-major change

Clarifying: is that a blocker?

happy to talk

I would love to talk to you about this 😄. Will reach out to see what works best.

recommendation is to stop using this rule

Since you're posting a recommendation I feel obliged to explain why this rule has stayed so rigid despite its wide use 🙂:

there are certain promises that must be awaited, and others that can be completely avoided.
allow maintainers to comment functions and let them bypass the rule

That's a problem that we've continuously bumped against in trying to make the rule palatable. The vast, vast majority of the time (>99%), Promises cannot be completely avoided - regardless of what the framework author says. If the framework has a bug then crashes in the "floating" Promise will be ignored by the calling code. It's almost always an indication of non-idiomatic code to see floating Promises.

Not saying your counterpoints aren't valid, but: there's a reason why this rule is so often used and recommended. So it'd be good for us to figure out if there's a good path forward!

[bradzacher]

there are certain promises that must be awaited, and others that can be completely avoided.

To be clear: the rule does not prevent usages of fire-and-forget promises.
By default it promotes a style of explicitly annotating these kinds of promises (void promise;). Such a style allows a developer to signal their intent - "I am not awaiting this promise on purpose" - unlike the alternative (promise;) where the intent is ambiguous to future readers.

The rule does not allow the maintainer/author of the library to indicate the distinction

My question as a user of any framework would be "if I'm not supposed to await this promise - why did you return one to me?".

If the response is "you need to await it sometimes, RTFM" - things are no longer statically clear and we've added nuance to the API usage. Nuance is not necessarily an issue!

Instead the issue is that neither JS nor TS has a way to describe a "fire and forget" promise - so it's really hard to create generic static analysis for this case. This is doubly true when frameworks don't have hard-and-fast rules for what is fire-and-forget and what is not.

---

You're presenting this problem very "black and white" however in reality the problem is quite "grey".

As Josh mentioned - from our user's experiences and from our own experiences:

• the vast majority of promises should be awaited
• a small percentage that should be not awaited, but should be .catched
• a tiny fraction of promises can be "forgotten" (not awaited or caught).

From my personal experience it's easy to drop a lot of cases into that last bucket accidentally (by just forgetting an await/catch) or on purpose (by misunderstanding the usecase).
For example I upgraded my company's codebase from node 14 to node 16+ - which turned on the (great) flag --unhandled-rejections=strict but meant I had to cleanup hundreds of such bad cases.

The lint rule is great at flagging the first two cases and has a mechanism to explicitly annotate the 3rd.

From a user's (and framework author's) perspective the missing piece is "automated" skipping of the 3rd case as opposed to manual. Personally I prefer the explicit opt-out, but I do understand why people would like an implicit one.

[bradzacher]

All that being said - we're open to an option.

Our response to feature requests in our repo is often terse - as you would understand as a maintainer of node responding deeply to issues takes time and as volunteers it's not always possible for us to dump all the necessary context when rejecting a request.

We're open but we're also very cautious about it. An option to ignore specific cases opens up a footgun in the rule - it opens up a big vector in a user's codebase.

If a framework has a truly "fire and forget" promise then there's no bug vector at all and allowing configuration to ignore it is good. Though one could argue (as mentioned above) why the framework even returns a promise if its never meant to be awaited - that is separate to the rule.

If the framework has a promise that is "sometimes F&F" and "sometimes await/catch" then always ignoring it is bad and is a footgun. False negatives are an insideous thing as they are invisible. We always prefer to create a small percentage of visible false positives rather than the same number of invisible false negatives.
FPs can be seen and acknowledged - even if they are annoying. FNs cannot be known until they cause a bug.
FNs also harm user trust in linting - a user thinks "It was not caught by the rule and caused a bug - what other cases were missed?". This has a negative impact on the entire ecosystem!

With a nuanced rule like no-floating-promise - I hope you can see why a false positive is generally better than a false negative.

---

Again all that being said - we're open to it, but we definitely want to be sure it's done for the right reasons and in a way that is less likely to cause false negatives.

[bradzacher]

I’m happy to talk to anybody on the tslint team about this

Nitpick. TSLint is dead and has been for over 5 years.
We are the typescript-eslint team -- and we are happy to talk to you at length 😄.

For reference - calling us the tslint team is like if we were to call you guys the io.js team - in a round-about way it's "correct", but it's far from the preferred name.

[mcollina]

For reference - calling us the tslint team is like if we were to call you guys the io.js team - in a round-about way it's "correct", but it's far from the preferred name.

I'm sorry about this, I didn't keep track!
Fun fact: the Node.js TSC email is [email protected], so calling us the io.js team might not be incorrect either!

---

From my personal experience it's easy to drop a lot of cases into that last bucket accidentally (by just forgetting an await/catch) or on purpose (by misunderstanding the usecase).
For example I upgraded my company's codebase from node 14 to node 16+ - which turned on the (great) flag --unhandled-rejections=strict but meant I had to cleanup hundreds of such bad cases.

The lint rule is great at flagging the first two cases and has a mechanism to explicitly annotate the 3rd.

What's the mechanism? Can we fix it in DefinitelyTyped (I'm a reviewer there) so everybody has a good experience? What's the point of changing the API?

@JoshuaKGoldberg pointed me to typescript-eslint/typescript-eslint#7008. I can't wait for this to land, so at least we will have a way to remove friction from developers. I hope this is also enabled by default by adding some kind of additional symbol/property so that frameworks can opt-in and solve this problem for their users.

---

I want to stress that the rule is black-and-white and put a drain on maintainers. I've been asked these questions a gazillion times and replied: "no, it's ok not to await those promises; we are handling it within the Framework."

Why such a convoluted APIs are needed? Because many Node.js APIs are not promise-friendly (starting from CommonJS). Specifically, there are complex issues when using promises with EventEmitter/Node.js Streams. Complex APIs with optional promises are needed to keep a nice DX for everybody, minus those who enable this rule (or use the default settings of typescript-eslint). For them, there is a lot of void to add everywhere, especially when using thenables/fluent APIs.

What's needed is an escape-hatch to allow maintainers to tell "it's ok to not await this promise/thenable", and turn that on by default.

---

The only alternative API design I could see for this issue is the following:

import { test, describe } from "node:test";

describe('something', async () => {
  await (test('aaa', async () => {
    // ...
  }).done)
})

I consider this a worse DX.

The alternative (which I pursued elsewhere) is not type the function as returning a Promise in the types. I think that's an option too, but it's a worse solution than having a nice escape hatch.

[targos]

There is a (documented) subtlety in Node.js API, which I believe is the source of this issue:

When you call test inside a describe, you are not supposed to await the returned promise (it's immediately resolved with undefined).

It's tricky because I don't think we can reflect that in the typings (make the function return a Promise or void depending on the context)?

[mcollina]

This requires await:

import { test } from 'node:test'
import {setTimeout} from 'node:timers/promises'

test('wrap', async t => {
  await test('some test', async t => {
    console.log('aa')
    await setTimeout(1000)
  })

  console.log('bb')

  await test('some test 2', async t => {
    console.log('cc')
    await setTimeout(1000)
  })
})

Output:

aa
bb
cc
▶ wrap
  ✔ some test (1003.013209ms)
  ✔ some test 2 (1002.693959ms)
▶ wrap (2011.603333ms)

ℹ tests 3
ℹ suites 0
ℹ pass 3
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 2016.177875

[bradzacher]

The lint rule is great at flagging the first two cases and has a mechanism to explicitly annotate the 3rd.
What's the mechanism?

The mechanism is on the user side - the void operator to signal "I am intentionally not awaiting this promise". This works fine for a lot of cases because "in general" the usecase of a promise you shouldn't await/catch ever is rarer.

That being said though that's the general case.
It's black-and-white from your perspective because your APIs are black-and-white.
If users are using a framework (like fastify or RTK) which is built on these "ignorable promises" then for them the rare case is the general case. That's what your users feel and it's why this is such a big pain point for you.
Note: not suggesting the style is bad -- just that it's not the "common case".

In contrast to the fastify case - the node:test case isn't black-and-white! There are times when you do want and need to await test, times you might want to, and times you don't want to. That's a grey case!

---

We generally build from the common case and then consider options for the less common cases as we receive user feedback.
Though there hasn't been too much feedback from users wanting an allowlist or similar - the intersection between (fastify, rtk, node:test) and (typescript-eslint, type-aware linting, no-floating-promises) is relatively small all things considered.

I say "small" but that may still be hundreds of thousands or a million users - which is less than 1/10th of our overall userbase. These scales are what can skew our perceptions - often in the wrong way. It's hard for us to gauge things - we're getting better about leaving things open to better gather and evaluate community feedback - in the past we were quicker to close things which further skewed things in the wrong ways by blocking additional feedback.

[mcollina]

@JoshuaKGoldberg is the branded check that you showed me possible in this case? reduxjs/redux-toolkit#4102. Alternatively, if the rule stopped complaining about Thenables, then we could type this as such.

Short term, I think those are the only solution.

I those are not enough, I think we could review a proposal that rework the API to not return a Promise, but it would require a deprecation cycle to essentially:

1. land the "new API" that allow the use case that I described above.
2. Backport the support for it on Node v20.
3. Deprecate awaiting the promise in v22/v23 (use a Thenable)
4. Remove returning the promise in v24.

This seems a lot of work that would need a champion.

---

A potential low cost alternative is to just modify the types in DefinitelyTyped to not return a promise. I've done this in the past and have stopped the issues about void.

[JoshuaKGoldberg]

This requires await:

// ...

test('wrap', async t => {
  await test('some test', async t => {
    console.log('aa')
    await setTimeout(1000)
  })

  console.log('bb')

  // ...
})

So if the user were to forget the first await and just call test('some test', ...) directly... that would be an undesired floating Promise? I.e. the rule is correct to report a violation in that case?

Not trying to be snarky. Just noting it seems that if users want to prioritize safety over convenience with the current design of the test API, they actually shouldn't allowlist/disable the rule. Am I misinterpreting?

is the branded check that you showed me possible in this case? reduxjs/redux-toolkit#4102

Yes! That work can happen in DefinitelyTyped, too - no changes needed to Node.js. I'll send a PR once we have support merged on our side, if nobody else has yet by then.

typescript-eslint/typescript-eslint#8088 (Docs: Overhaul no-floating-promise messages and docs) tracks our reworking of Promise-related docs. I just added a note to make sure we remember to also document these strategies.

the rule stopped complaining about Thenables

👍! Filed typescript-eslint/typescript-eslint#8433. It has context -partially overlapping with this thread- as to how Thenable-checking came to be the default back in the day.

Once typescript-eslint/typescript-eslint#7008 (Enhancement: [no-floating-promises] add an 'allowForKnownSafePromises' option) is implemented, I think a followup we could consider is having the rule default to allowing any type extending a typed named something special like SafePromise or imported from some special @typescript-eslint/userland-types package... but I think we'd want to see if the existing set of proposed improvements are enough for users. We're generally loath to add name-based hardcodings and/or ask folks to install any more of our packages than they need.

potential low cost alternative is to just modify the types in DefinitelyTyped to not return a promise

That could work for this specific issue. I do worry about making the types not reflective of reality, though. When folks actually do want the Promise behavior -which #51292 (comment) indicates is a real user story- then that void will be very confusing. And it'd cause violations with @typescript-eslint/await-thenable!

This seems a lot of work that would need a champion.

I've always wanted to champion something in Node.js... But no, especially given the above "This requires await note", I'm not even positive I want that to happen! As with the SafePromise overrides, I'm personally leaning towards trying to fix it with safe Promise branding & rule options.

---

Btw, for anybody who's lost in the 3k+ words at this point 😄, here is the current list of feature requests on the typescript-eslint side for helping make this less painful:

• Enhancement: [no-floating-promises] add an 'allowForKnownSafePromises' option typescript-eslint/typescript-eslint#7008
• Docs: Overhaul no-floating-promise messages and docs typescript-eslint/typescript-eslint#8088
• Enhancement: [no-floating-promises] add an 'allowForKnownSafeCalls' option typescript-eslint/typescript-eslint#8404
• Enhancement: [no-floating-promises] Don't check coincidentally Promise-like (Thenable) type values by default typescript-eslint/typescript-eslint#8433

[mcollina]

So if the user were to forget the first await and just call test('some test', ...) directly... that would be an undesired floating Promise? I.e. the rule is correct to report a violation in that case?

The test will likely fail. I don't see how the rule adds anything in this case.
However, that promise must be awaited.

Not trying to be snarky. Just noting it seems that if users want to prioritize safety over convenience with the current design of the test API, they actually shouldn't allowlist/disable the rule. Am I misinterpreting?

The error will be caught anyway by a failing test. There is no risk of a "floating promise" error situation here. Regarding safety, node:test takes care of itself and errors if there are any "floating promises" remaining. It's quite helpful.

The interesting problem is that it's unnecessary elsewhere, and the most common case is completely safe to ignore. Folks can await the tests if needed for various reasons, but it's not strictly needed.

[cjihrig]

There appears to be some confusion in this discussion. I will try to explain the current state of things as I understand them:

• test() returning a Promise was an intentional design decision simply for knowing when a piece of asynchronous code finished executing. As far as I know, users never need to do anything with its return value unless they are doing it for some custom control flow purpose.
• t.test() only exists inside of a test or subtest. I think this is where the bulk of the confusion comes from. This promise should be handled by the user. This was initially inspired by Deno's concept of test steps.

This requires await:

import { test } from 'node:test'
import {setTimeout} from 'node:timers/promises'

test('wrap', async t => {
  await test('some test', async t => {
    console.log('aa')
    await setTimeout(1000)
  })

  console.log('bb')

  await test('some test 2', async t => {
    console.log('cc')
    await setTimeout(1000)
  })
})

Output:

aa
bb
cc
▶ wrap
  ✔ some test (1003.013209ms)
  ✔ some test 2 (1002.693959ms)
▶ wrap (2011.603333ms)

ℹ tests 3
ℹ suites 0
ℹ pass 3
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 2016.177875

That example does require awaiting the nested test() functions, but you're really supposed to be using t.test() in those cases. If you're looking to classify this as "black and white" for linting purposes, I think there are two different ways of doing it:

1. test() can be ignored. t.test() should be handled. This is probably the easiest to implement for a linter.
2. Inside of a test() function, promises should be handled. Outside of a test() function you shouldn't need to.

[kayahr]

In this issue the allowForKnownSafePromises option for @typescript-eslint/no-floating-promises is mentioned several times. Can someone tell me how to use it correctly with node:test to get rid of the warnings? In my understanding the name property must specify the type name, so even when node:test does not yet use some special SafePromise type I think it should work like this to ignore all returned promises from the node:test module:

"allowForKnownSafePromises": [
    {
        "from": "package",
        "name": "Promise",
        "package": "node:test"
    }
]

But it doesn't. In some proposal for this feature it was mentioned to specify the function names, which return the promises to be ignored, so I also tried "name": [ "it", "describe" ], which also doesn't work.

It works when doing it on a global level like this:

"allowForKnownSafePromises": [
    {
        "from": "lib",
        "name": "Promise"
    }
]

But this is obviously a bad idea because it matches every Promise and renders the whole eslint rule useless.

Am I doing something wrong or is there currently no solution at all to disable the floating-promise warnings for it and describe (except prefixing them all with await or void)? If there is no better solution I might write my own it/describe wrapper functions which voids the promises centrally so all the test files don't have to do that. But would be nicer if it worked out-of-the box without writing workarounds.

[kayahr]

To answer my own question: I mixed up the options allowForKnownSafePromises and allowForKnownSafeCalls. The first one is present in current eslint typescript plugin and I can't get it to work, The second is currently only present in the 8.0.0-alpha versions of the eslint plugin and work fine with this rule:

"@typescript-eslint/no-floating-promises": [
    "warn",
    {
        "allowForKnownSafeCalls": [
            {
                "from": "package",
                "name": [ "it", "describe" ],
                "package": "node:test"
            }
        ]
    }
]

[benjamingr]

I think the no-floating-promises rule is problematic and sincerely limit the capability of frameworks.

I think it needs more nuance to be generally useful. When writing scripts and tests it's often but not always meaningless to wait for the result because that's "the thing you're executing".

This has been true since we started crashing on unhandled rejections which made code safe and the lint rule redundant for these cases.

void it("does something", () => {

});

Is as pointless as

void (async () => {
  // ..
})(); 

In a script. In particular, I think the rule makes the most sense in nested contexts though it's very easy to see a case where it's useful in the top scope or not useful in a nested scope.

[bradzacher]

I think it needs more nuance to be generally useful.

From my experience turning it on in large codebases - this is not correct. Cases of infallible promises / promises that are optionally awaitable are the exception and from experience they're actually relatively rare.

Source: we have had this rule in our recommended set (for as long as we've had a recommend config!) and we have not had many issues filed. So either users are suffering in silence or my interpretation above is correct.

The issue is that if your codebases opts into a tool that uses such a pattern then it is immediately becomes a major pain point for you. I.e. If you opt-in to node:test then the pattern will occur in every single test file in your codebase.

Put another way - generally the rule is useful, but when you use a case as described it can be frustrating to work with.
Which is why we are adding options to try to make those cases easier to work with - to allow people to configure these specific cases as safe to ignore.

[felixfbecker]

Apologies if I'm missing something obvious, but is there any reason why we can't just always await the result to test()? It seems the easiest and safest way to avoid issues and not have the lint rule trigger would be to just always await:

import { test } from "node:test"

await test("foo", async (t) => {
  await t.test("bar", () => {

  })
})

That's what I've been doing in all my tests and it seems to work fine.

Top-level await has been available in Node for a long time now.

[benjamingr]

From my experience turning it on in large codebases - this is not correct. Cases of infallible promises / promises that are optionally awaitable are the exception and from experience they're actually relatively rare.

That's fine, it's good we have differing experience. I've worked on promises in Node and userland (including libraries that pioneered these sort of errors) for more than 10 years at this point. I've turned this rule on and off at startups and at codebases with 10M+ LoC at companies like Microsoft.

The fact we have differing experiences is good, I acknowledge your experience is as valid as mine and you have a lot of experience as a maintainer of the rule.

I am happy to name at least 10 cases similar to this where not awaiting the promise at the top level is useful.

I'm also really enjoying the irony of arguing the "floating promises is fine sometimes" case after specifying the hook and fighting for the heuristic that makes not .catching a promise crash the process :) This is the first time I'm on this side of this discussion.

Source: we have had this rule in our recommended set (for as long as we've had a recommend config!) and we have not had many issues filed. So either users are suffering in silence or my interpretation above is correct.

I think most people don't really understand the rule or nuance and the rule predates not awaiting top level promises being safe.

---

Apologies if I'm missing something obvious, but is there any reason why we can't just always await the result to test()

Users likely won't do this in practice though :]