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

[checks] Expectations that return futures do not play well with the unawaited_futures lint #2139

Open
Quijx opened this issue Nov 17, 2023 · 2 comments
Assignees
Labels
package:checks Issues related to pkg:checks

Comments

@Quijx
Copy link

Quijx commented Nov 17, 2023

For example when testing a stream using exactly the way it is described in the README, it triggers the unawaited_futures lint:

import 'package:checks/checks.dart';
import 'package:test/test.dart';

void main() {
  test('test', () async {
    final stream = Stream.fromIterable([1, 2, 3]);

    await check(stream).withQueue.inOrder([
      it()..emits(it()..equals(1)),
      it()..emits(it()..equals(2)),
      it()..emits(it()..equals(3)),
      it()..isDone(),
    ]);
  });
}

Or as image to see the lints:
image

Applying the auto fix produces invalid code:

await check(stream).withQueue.inOrder([
  it()await ..emits(it()..equals(1)),
  it()await ..emits(it()..equals(2)),
  it()await ..emits(it()..equals(3)),
  it()await ..isDone(),
]);

I also don't think there is a way to add awaits here that satisfy the lint.
There are probably other situations in which future returning expectations cause similar problems.

Brainstorming Possible Solutions

One solution would be do change the lint, so that it does not trigger in cascades, but somehow also seems wrong, since there actually are unawaited futures.

Maybe expectations should not return futures, but instead add them to the context? Then the Subject could be a awaited or example by doing:

await completionOf(
  check(stream)
    ..withQueue.inOrder([
      it()..emits(it()..equals(1)),
      it()..emits(it()..equals(2)),
      it()..emits(it()..equals(3)),
      it()..isDone(),
    ]),
);

This would then await the completion of the entire check.
Not sure if that's good though.

@Quijx Quijx added the package:checks Issues related to pkg:checks label Nov 17, 2023
@Quijx
Copy link
Author

Quijx commented Nov 20, 2023

I just noticed, that with the changes from the master branch this does not seem to be a problem:
image
But I must say that I am not a fan of the new callback syntax, especially when using it with the usually pretty awesome Type Inlay Hints from the Flutter Enhancement Suite plugin. This makes this very visually cluttered. But even without the type hints, I think repreating the it twice is a bit verbose. This is amplified by the expectations for streams already being relatively verbose, but that is arguably a separate issue :)

@natebosch
Copy link
Member

But I must say that I am not a fan of the new callback syntax

The Condition syntax got some feedback that it was confusing. I didn't get much positive feedback about the syntax (although I was a fan of the reduced noise and held on to it for a while) so I ended up dropping it. It was a low volume of feedback, and this positive feedback pushes closer to neutral overall.

Applying the auto fix produces invalid code:

Thanks for pointing this out. Filed dart-lang/sdk#54105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:checks Issues related to pkg:checks
Projects
None yet
Development

No branches or pull requests

2 participants