Re: Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

[Kris Maglione]

On Thu, Oct 31, 2019, 06:02 Jason Orendorff  wrote:

> Ignoring the awaited value here is like using `catch {}` to squelch all
> exceptions, or ignoring the return value of an async function or method, or
> any other expression that produces a Promise. Do we have lints for those
> pitfalls? I'm kind of surprised that there doesn't seem to be a built-in
> ESLint lint for `catch {}`.
>

Some directories have an ESLint rule to disallow empty blocks, which
requires that any empty catch blocks at least contain a comment to justify
them.

>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

[Jason Orendorff]

On Thu, Oct 31, 2019 at 4:10 AM Paolo Amadini 
wrote:

>// INCORRECT
>//await Promise.allSettled([promise1, promise2]);
>
> The last example silently loses any rejection state.
>

Ignoring the awaited value here is like using `catch {}` to squelch all
exceptions, or ignoring the return value of an async function or method, or
any other expression that produces a Promise. Do we have lints for those
pitfalls? I'm kind of surprised that there doesn't seem to be a built-in
ESLint lint for `catch {}`.

There's nothing special about tests here, right? Those patterns are fishy
whether you're in a test or not.

-j
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

[Paolo Amadini]

On 10/30/2019 10:19 PM, Jan-Ivar Bruaroey wrote:

This always seemed trivial to me to do with:
     Promise.all(promises.map(p => p.catch(e => e)))


As Boris pointed out, this does not have proper exception handling. If
exceptions should be ignored, it may be good to call "console.error". In
case exceptions should be fatal, like in most automated test files in
mozilla-central, something like the following would be better instead:

  .catch(ex => ok(false, ex))

But unless you have an array to begin with, in test files it's generally
better to "await" on each promise. There is a danger with allSettled:

  // Better
  await promise1;
  await promise2;

  // Also valid
  await Promise.all([promise1, promise2]);

  // INCORRECT
  //await Promise.allSettled([promise1, promise2]);

The last example silently loses any rejection state.

My recommendation to avoid this pitfall would be to add an ESLint rule.
However, I don't know how sophisticated they could be, and just checking
that the caller does something with the return value may not be enough,
if passing it to "await" or calling ".then" is enough to silence the
warning. Maybe just disallow allSettled in tests unless overridden?

Cheers,
Paolo
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform