Return a Future from expect(). #529

nex3 · 2017-02-01T20:48:09Z

No description provided.

This also adds a (private) AsyncMatcher class that makes it easier to define asynchronous matchers, and generally improves the formatting of those matchers.

efortuna · 2017-02-01T23:20:26Z

lib/src/frontend/expect.dart

+  buffer.writeln(indent(prettyPrint(expected),       first: 'Expected: '));
+  buffer.writeln(indent(prettyPrint(actual),         first: '  Actual: '));
+  if (which.isNotEmpty) buffer.writeln(indent(which, first: '   Which: '));
+  if (reason != null) buffer.writeln(reason);


did you want to indent the "reason" line too?

No, the extra whitespace is just to visually verify that the first prefixes are all the same width.

efortuna · 2017-02-01T23:22:09Z

lib/src/frontend/future_matchers.dart

-    Invoker.current.addOutstandingCallback();
+  // Avoid async/await so we synchronously start listening to [item].
+  /*FutureOr<String>*/ matchAsync(item) {
+    if (item is! Future) return "was not a Future";


why this string here but null below?

A string means failure, null means success. It's a failure if the item isn't the right type, but if _matcher is null that just means that this came from completes and doesn't care about the actual completion value.

gotcha. can you add that to the comments?

It's in the AsyncMatcher.matchAsync() docs—is it really worth duplicating in the subclasses?

oh. right. I can read. Carry on then!

efortuna · 2017-02-01T23:31:17Z

lib/src/utils.dart

+/// [size] defaults to `first.length`. Otherwise, [size] defaults to 2.
+String indent(String string, {int size, String first}) {
+  size ??= first == null ? 2 : first.length;
+  return prefixLines(string, " " * size, first: first);


hey I didn't know we could do the " " * n thing in dart, too! Cool!

Yeah, it's so handy it seems out-of-place 😝.

efortuna · 2017-02-01T23:34:38Z

lib/src/utils.dart

+  if (lines.length == 1) return "$single$text";
+
+  var buffer = new StringBuffer("$first${lines.first}\n");
+  for (var line in lines.skip(1).take(lines.length - 2)) {


this line with the skip and take len - 2 is a little complex. Consider adding a comment describing why it's like this

efortuna

approve after one comment addition

efortuna · 2017-02-02T18:24:30Z

lib/src/frontend/future_matchers.dart

-    Invoker.current.addOutstandingCallback();
+  // Avoid async/await so we synchronously start listening to [item].
+  /*FutureOr<String>*/ matchAsync(item) {
+    if (item is! Future) return "was not a Future";


gotcha. can you add that to the comments?

natebosch · 2017-02-11T01:28:53Z

This interacts really poorly with http://dart-lang.github.io/linter/lints/unawaited_futures.html

When should/shouldn't a caller await the returned Future? Should we be littering await everywhere just in case it's one of the places we need it?

kevmoo · 2017-02-13T20:39:12Z

@nex3 Thoughts on this? I saw many things break.

Could this be a generic method maybe? Then matchers can opt to have a return value or –– or something...

nex3 · 2017-02-13T21:42:09Z

This interacts really poorly with http://dart-lang.github.io/linter/lints/unawaited_futures.html

It seems like a flaw in the lint if it can't handle Futures that don't need to be listened to.

When should/shouldn't a caller await the returned Future? Should we be littering await everywhere just in case it's one of the places we need it?

It returns a Future that completes when the matcher is finished running. A test should await this future if and only if it wants to block the execution of the test of the test on a given matcher. It certainly shouldn't await every expect()—if the matcher is synchronous, it will just return a completed Future.

Thoughts on this? I saw many things break.

Do you mean dart-lang/sdk#57503? That's just a bug that I'll fix. This should be a fully backwards-compatible change in terms of Dart semantics.

Could this be a generic method maybe? Then matchers can opt to have a return value or –– or something...

This would be a breaking change both for expect() and for the Matcher API. It also wouldn't really solve the issue, since most of the time you probably don't want to await an expect() even if it does have an asynchronous matcher.

natebosch · 2017-02-13T21:45:27Z

If we ignore the lint this seems like a usability problem - We've had folks ask in gitter whether they should be awaiting the Future. Unless we're prepared to tell people "Every expect should have an await" there is going to be a lot of confusion.

kevmoo · 2017-02-13T21:46:48Z

It's not just lints. ``` void expectFoo() => expect('foo', isNotNull); ``` Is a non-lint error because you're (now) trying to return a Future from a void function.

…

On Mon, Feb 13, 2017 at 1:45 PM, Nate Bosch ***@***.***> wrote: If we ignore the lint this seems like a usability problem - We've had folks ask in gitter whether they should be awaiting the Future. Unless we're prepared to tell people "Every expect should have an await" there is going to be a lot of confusion. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCinriwq5cPSFgWWggaObQbpWFwhF7ks5rcM73gaJpZM4L0XfK> .

natebosch · 2017-02-13T21:48:09Z

since most of the time you probably don't want to await an expect() even if it does have an asynchronous matcher

What is a case where you do want to await?

matanlurey · 2017-02-13T21:55:28Z

+1. This is going to make practically every internal user unhappy.

I'm not comfortable with this landing.

nex3 · 2017-02-13T22:06:23Z

If we ignore the lint this seems like a usability problem - We've had folks ask in gitter whether they should be awaiting the Future. Unless we're prepared to tell people "Every expect should have an await" there is going to be a lot of confusion.

I'm happy to update the method documentation if you have suggestions. The API is necessary, though—stream matchers aren't usable without it.

It's not just lints.
void expectFoo() => expect('foo', isNotNull);
Is a non-lint error because you're (now) trying to return a Future from a
void function.

I've always considered adding a return type to a previously void method to be a backwards-compatible change. If this means it's not backwards-compatible, that's a broader problem than just expect(). We should either update the language to allow the case above or we should forbid the use of void ... => entirely so that APIs have the freedom to add return types.

I've filed #28763 to track the broader issue.

What is a case where you do want to await?

Any time you want to wait until the matcher is complete. For example, you might await expect(foo.bar(), throwsStateError) to ensure that the function is no longer running before you continue the test. Or you might await expect(process.stdout, emits("Ready!")) so that you continue the test after the process is ready.

natebosch · 2017-02-13T22:16:24Z

Given that we likely can't address the SDK issue in the short term what is our plan?

nex3 · 2017-02-13T22:22:45Z

My suggestion is to address dart-lang/sdk#28763 ASAP in Google3 so we can start rolling test again. Externally, this is already released so there's not much we can do other than try to get the fix for dart-lang/sdk#28763 into 1.23.

kevmoo · 2017-02-13T22:24:00Z

We could back out this change – or make expect return dynamic – or look at a generic method annotation.

natebosch · 2017-02-13T22:26:20Z

The SDK issue is very unlikely to be solvable in the short term - we're not going to find a solution very many people are happy with. If we consider ourselves blocked on that I suspect it's going to be a long time before we can roll test internally.

nex3 · 2017-02-13T22:36:16Z

We could back out this change – or make expect return dynamic – or look at a generic method annotation.

Both backing out the change and adding a generic method annotation would themselves be backwards-incompatible. Making expect return dynamic wouldn't solve the issue, since void foo() => expect(...) would still fail at runtime.

This is also a key aspect of test's support for testing asynchronous APIs. This is the right API for doing so, and if I had to increment the major version to make this change I'd do it. We need stream matchers, and we need them to integrate well with expect().

The SDK issue is very unlikely to be solvable in the short term - we're not going to find a solution very many people are happy with.

Why? What's to be unhappy about with loosening the restriction on using => with a void return type?

If fixing the language issue isn't workable in the short term, we should find a way to auto-refactor the Google3 codebase to change => expect to { expect }, as we would for any breaking change.

natebosch · 2017-02-13T22:37:17Z

What are your thoughts on a separate expectAsync method?

nex3 · 2017-02-13T22:40:33Z

That seems pretty confusing, since the various asynchronous matchers are totally usable with expect(). Doing that instead of the current solution would also be backwards-incompatible in the external world.

natebosch · 2017-02-13T22:49:22Z

We're already backwards incompatible in the external world for any code using the pattern mentioned right?

I think we should separately consider the API we want to end up with and the path we take to get there - apologies that I've been threading those two conversations throughout.

Are there examples of code that are mixing and matching between async and non-async matchers? I'd be curious to play around with what it would look like if we forced them to be explicitly chosen. It'd be good to get more data on what is more confusing - picking the right expect or seeing it return a Future and not knowing what to do with it.

We should also experiment with a generic method... though I suspect the current flexibility of taking a value or a matcher is very likely to rule that out.

nex3 · 2017-02-13T23:08:43Z

We're already backwards incompatible in the external world for any code using the pattern mentioned right?

Tragically yes, but that was unintentional and we've already paid much of the (external) cost for it. This would be an intentional backwards incompatibility. Also, because await expect(...) would still be valid, this backwards incompatibility would be much more likely to cause subtle bugs.

I think we should separately consider the API we want to end up with and the path we take to get there - apologies that I've been threading those two conversations throughout.

I'm confident that the API in the most recent release is the API we should end up with. The documentation could probably use a little work, though.

Are there examples of code that are mixing and matching between async and non-async matchers? I'd be curious to play around with what it would look like if we forced them to be explicitly chosen.

All the code I know of that would use this pattern hasn't been ported away from scheduled_test yet.

natebosch · 2017-02-13T23:25:15Z

I'm confident that the API in the most recent release is the API we should end up with.

What data are you using? It's come up externally that the question of whether to await is confusing. What evidence do we have that people will find it more confusing to need to explicitly opt-in if they want to be able to await the result?

All the code I know of that would use this pattern hasn't been ported away from scheduled_test yet.

Is that evidence that little code will be impacted by the breaking change of rolling this back?

natebosch · 2017-02-13T23:44:20Z

Usage of the void foo() => expect(...) pattern is very low. 2 usages have already been fixed externally and I only see 2 remaining that we're likely to care about

natebosch · 2017-02-14T00:53:43Z

Some data so we can decide if we want to ignore the unawaited_futures lint:

Internally ~35% of projects using linting are using unawaited_futures, ~15% of projects with an analysis_options are using it.

On github it's harder to tell, ~15% of projects using linting have "unawaited_futures" in their analysis_options but some fraction of that is commented out to disable the lint.

nex3 · 2017-02-14T22:49:48Z

What data are you using? It's come up externally that the question of whether to await is confusing. What evidence do we have that people will find it more confusing to need to explicitly opt-in if they want to be able to await the result?

I'm using my years of highly successful API design experience. I don't believe there's an effective way to empirically measure quality design: it's much more of an art than a science. I'm happy to listen to feedback though—in this case, I plan to improve the documentation to explain more clearly when an expect() needs to be awaited based on feedback.

zoechi · 2017-02-15T07:40:39Z

I find the ~~await_only_futures~~unawaited_futures lint quite useful and usually there are only few exceptions where I suppress the warning.
There were extensive discussions how to make that work without too many false positives, but with this change it becomes unbearable.

tvolkert · 2017-02-15T19:27:11Z

I find the await_only_futures lint quite useful and usually there are only few
exceptions where I suppress the warning.

+1. It's caught real bugs in our codebase in the past, but with the update to package:test, I had to turn it off due to a firehose of lint warnings (dart-lang/sdk#57437).

nex3 · 2017-02-15T23:20:57Z

I'm confident we can get the linter to work well with this change soon.

cbracken · 2017-02-17T23:29:55Z

It looks like this may have caused one of the fake_async tests to start failing in quiver. Wondering how broadly this'll affect users of that or similarly zone-reliant tests.

nex3 · 2017-02-17T23:32:07Z

If this interacts any differently with zones than the old version, that's a bug. Let me know if you can find a reproduction.

tvolkert · 2017-02-17T23:36:04Z

Confirmed that it seems to be interacting with zones differently.

Similar to the fake_async failures, I discovered that this change causes Flutter tests to fail because they make expectations about the number of pending microtasks in the queue -- and now expect() queues a microtask.

@Hixie

cbracken · 2017-02-17T23:37:01Z

Repro:

git clone [email protected]:google/quiver-dart.git
cd quiver-dart
pub get
pub run test test/testing/async/fake_async_test.dart

Result:

00:00 +37 -1: FakeAsync stats should report the number of pending microtasks
  Expected: <1>
    Actual: <2>

  package:test                                             expect
  test/testing/async/fake_async_test.dart 508:11           main.<fn>.<fn>.<fn>.<fn>
  package:quiver/testing/src/async/fake_async.dart 184:43  _FakeAsync.run.<fn>
  dart:async                                               _CustomZone.runGuarded
  package:quiver/testing/src/async/fake_async.dart 184:18  _FakeAsync.run
  test/testing/async/fake_async_test.dart 505:25           main.<fn>.<fn>.<fn>

Then edit pubspec.yaml to use package test 0.12.18, pub get, and re-run:

00:00 +41: All tests passed!

It's entirely possibly this is a bug in FakeAsync, or if not, that we could put special-case logic in for package:test which, given its relative ubiquity, seems reasonable. I'm less concerned about this particular case than making sure there's not any broader impact where the failure mode will be tricky to diagnose for impacted developers.

nex3 · 2017-02-17T23:45:20Z

@tvolkert That's not a safe assertion to make in general—microtasks are an implementation detail, and any library (including core libraries) could queue more or less at any time. The event loop should be considered a black box.

@cbracken I can chase that down if I need to, but it would be really helpful if you could find a standalone reproduction.

tvolkert · 2017-02-17T23:49:39Z

microtasks are an implementation detail

Here, we're trying to test the implementation, and since we are the Dart VM embedder, such details are very valid to test.

nex3 · 2017-02-17T23:55:22Z

I suppose, but you're testing something very contingent on the implementations of every library those tests touch, which means you'll need to be prepared to update those assertions when you get new upstream versions.

Hixie · 2017-02-18T00:04:56Z

The problem is that we are mostly trying to test whether a microtask got queued at all or not. If we start having to verify whether a single microtask got queued or not that gets much harder to maintain.

Hixie · 2017-02-18T00:06:23Z

Would it be possible to have an expectSync() method that doesn't handle being given a future at all, and doesn't return a future?

We're already wrapping expect() and test() anyway so it would be trivial for us to just switch to a less "clever" API on the back end.

cbracken · 2017-02-18T00:07:39Z

@nex3 reduced repro:

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

class FakeAsync {
  final _microtasks = [];
  Zone _zone;

  int get microtaskCount => _microtasks.length;

  run(callback(FakeAsync self)) {
    if (_zone == null) {
      _zone = Zone.current.fork(specification: new ZoneSpecification(
          scheduleMicrotask: (_, __, ___, Function microtask) {
        _microtasks.add(microtask);
      }));
    }
    return _zone.runGuarded(() => callback(this));
  }
}

main() {
  test('count pending tasks', () {
    new FakeAsync().run((fakeAsync) {
      expect(fakeAsync.microtaskCount, 0);
      scheduleMicrotask(() => null);
      expect(fakeAsync.microtaskCount, 1);
      // passes under test 0.12.18
      // fails under test 0.12.19: expected 1, actual 2 
    });
  });
}

Under the assumption that expect now throws a microtask in the queue, this is obviously right, but it's a relatively significant breaking change for tests using FakeAsync and potentially other similar test libraries.

In particular, there may be some test frameworks that test that try to blast through the set of pending async tasks until there are none, then test that certain conditions are met. If the test expectations are also queued up in there, this could be problematic.

Hixie · 2017-02-18T00:16:53Z

One thing to note is that Flutter can't actually await the Future that's returned by expect anyway, since we're running in a FakeAsync zone and thus microtasks don't trigger until we flush them. So for example the code await expect(false, isFalse) would always hang. We do use Futures in our test harness but we have to very carefully generate them from different zones.

This is quite intentional, because we want to test code that uses futures but we want to very carefully step them through state machines in controlled ways (so we can verify different potential race conditions, for example). We also use the lack of any microtasks as a signal that we're not leaking asynchronous work in certain cases; the code that verifies that doesn't know if you've called expect or not first.

nex3 · 2017-02-18T00:22:56Z

The problem is that we are mostly trying to test whether a microtask got queued at all or not. If we start having to verify whether a single microtask got queued or not that gets much harder to maintain.

That's kind of the position you're in if you're writing a test like that... making assertions about implementation details is never going to be easy to maintain. I know the core Dart SDK platform tests avoid the test package largely because they're testing very sensitive behavior like this. I'm honestly surprised you haven't run into issues before from the heartbeat function or other test internals.

That said, in this case in particular since we're just returning new Future.value() it should be possible to cache that at the top level without disrupting the API.

@cbracken If the issue is just with a microtask counter, the answer's the same as for Flutter: these tests are relying on implementation details of the libraries they're using and the runtime, and should expect to be broken occasionally.

nex3 added 4 commits February 1, 2017 12:46

Return a Future from expect().

b5ff3af

This also adds a (private) AsyncMatcher class that makes it easier to define asynchronous matchers, and generally improves the formatting of those matchers.

Update existing tests.

fa77869

Add new tests.

8fca485

Update pubspec and changelog.

368cc19

googlebot added the cla: yes label Feb 1, 2017

nex3 requested a review from efortuna February 1, 2017 20:55

efortuna suggested changes Feb 1, 2017

View reviewed changes

code review changes

f22f10e

efortuna approved these changes Feb 2, 2017

View reviewed changes

nex3 merged commit ca2faf5 into master Feb 2, 2017

nex3 deleted the expect-future branch February 2, 2017 22:06

zoechi mentioned this pull request Feb 14, 2017

Spurious unawaited_futures warnings dart-lang/sdk#57437

Closed

tvolkert mentioned this pull request Feb 15, 2017

enable unawaited_futures analysis option flutter/flutter#5793

Closed

natebosch mentioned this pull request May 9, 2018

Use a new future instead of a cached future from another zone #831

Merged

Return a Future from expect(). #529

Return a Future from expect(). #529

Conversation

nex3 commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natebosch commented Feb 11, 2017

kevmoo commented Feb 13, 2017

nex3 commented Feb 13, 2017

natebosch commented Feb 13, 2017

kevmoo commented Feb 13, 2017 via email

natebosch commented Feb 13, 2017

matanlurey commented Feb 13, 2017

nex3 commented Feb 13, 2017 • edited by natebosch Loading

natebosch commented Feb 13, 2017

nex3 commented Feb 13, 2017 • edited Loading

kevmoo commented Feb 13, 2017

natebosch commented Feb 13, 2017

nex3 commented Feb 13, 2017

natebosch commented Feb 13, 2017

nex3 commented Feb 13, 2017

natebosch commented Feb 13, 2017

nex3 commented Feb 13, 2017 • edited Loading

natebosch commented Feb 13, 2017

natebosch commented Feb 13, 2017

natebosch commented Feb 14, 2017

nex3 commented Feb 14, 2017

zoechi commented Feb 15, 2017 • edited Loading

tvolkert commented Feb 15, 2017

nex3 commented Feb 15, 2017

cbracken commented Feb 17, 2017

nex3 commented Feb 17, 2017

tvolkert commented Feb 17, 2017

cbracken commented Feb 17, 2017 • edited Loading

nex3 commented Feb 17, 2017

tvolkert commented Feb 17, 2017

nex3 commented Feb 17, 2017

Hixie commented Feb 18, 2017

Hixie commented Feb 18, 2017 • edited Loading

cbracken commented Feb 18, 2017 • edited Loading

Hixie commented Feb 18, 2017

nex3 commented Feb 18, 2017

nex3 commented Feb 13, 2017 •

edited by natebosch

Loading

nex3 commented Feb 13, 2017 •

edited

Loading

nex3 commented Feb 13, 2017 •

edited

Loading

zoechi commented Feb 15, 2017 •

edited

Loading

cbracken commented Feb 17, 2017 •

edited

Loading

Hixie commented Feb 18, 2017 •

edited

Loading

cbracken commented Feb 18, 2017 •

edited

Loading