More tests for async* functions

More tests for async* functions.

An async* stream may only be canceled at yield-points.

R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/310385aeb2b007774cf90dd6e965dfd2861f915a

[floitsch, 2016-02-08 17:30:01]

Description was changed from

==========
Fix cancel on streams that are waiting at an await.
==========

to

==========
Fix cancel on streams that are waiting at an await.

BUG= http://dartbug.com/23436   (the dart2js part of it).
==========

[floitsch, 2016-02-08 17:30:36]

floitsch@google.com changed reviewers:
+ sigurdm@google.com

[Lasse Reichstein Nielsen, 2016-02-08 17:44:23]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2016-02-08 17:44:24]

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:264: /// completes.
That's not correct.
Canceled async* functions must only (attempt to) exit at a yield, not at an
await. That's documented above. It *must not* exit at an await.

[Lasse Reichstein Nielsen, 2016-02-08 20:54:44]

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:264: /// completes.
(or maybe I'm not understanding exactly what happens when this function is
called).

[Lasse Reichstein Nielsen, 2016-02-08 21:04:35]

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
File tests/language/async_star_cancels_after_await_test.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
tests/language/async_star_cancels_after_await_test.dart:27: counter++;
should there be a yield here?

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
File tests/language/async_star_no_cancel_test.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
tests/language/async_star_no_cancel_test.dart:23: counter++;
Should there be a yield here?

Otherwise you have an async* function with no yield/yield* which means it is
unaffected by cancel.

[floitsch, 2016-02-09 16:54:58]

Description was changed from

==========
Fix cancel on streams that are waiting at an await.

BUG= http://dartbug.com/23436   (the dart2js part of it).
==========

to

==========
More tests for async* functions.

An async* stream may only be canceled at yield-points.
==========

[floitsch, 2016-02-09 16:56:34]

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:264: /// completes.
On 2016/02/08 17:44:24, Lasse Reichstein Nielsen wrote:
> That's not correct.
> Canceled async* functions must only (attempt to) exit at a yield, not at an
> await. That's documented above. It *must not* exit at an await.

My fault. I reverted these changes.

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
File tests/language/async_star_cancels_after_await_test.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
tests/language/async_star_cancels_after_await_test.dart:27: counter++;
On 2016/02/08 21:04:35, Lasse Reichstein Nielsen wrote:
> should there be a yield here?

No.
I thought that async* functions can be canceled at await-points, too.

Modified this test to check that we only cancel at yield points (or never, if
there isn't any).

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
File tests/language/async_star_no_cancel_test.dart (right):

https://codereview.chromium.org/1677063003/diff/20001/tests/language/async_st...
tests/language/async_star_no_cancel_test.dart:23: counter++;
On 2016/02/08 21:04:35, Lasse Reichstein Nielsen wrote:
> Should there be a yield here?
> 
> Otherwise you have an async* function with no yield/yield* which means it is
> unaffected by cancel.

Yes. The tests now make sure that there is no cancel, if there isn't any
`yield`.

[floitsch, 2016-02-09 18:48:04]

floitsch@google.com changed reviewers:
+ hausner@google.com
- lrn@google.com, sigurdm@google.com

[floitsch, 2016-02-10 20:43:04]

ping. (@Matthias I made you the main reviewer).

[hausner, 2016-02-10 21:13:17]

Ah, didn't realize I was the main reviewer.

LGTM w/comments.

It would also be nice to describe how the two tests are different. Looks to me
that they are both checking the same thing.

https://chromiumcodereview.appspot.com/1677063003/diff/60001/tests/language/a...
File tests/language/async_star_no_cancel_test.dart (right):

https://chromiumcodereview.appspot.com/1677063003/diff/60001/tests/language/a...
tests/language/async_star_no_cancel_test.dart:40: subscription.cancel().then((_)
=> cancelFinished = true);
Maybe add and event here that proves that the subscription was cancelled. Then
check for it below. That will catch if callbacks are not executed in the
expected order.

[floitsch, 2016-02-11 13:53:26]

Added more descriptive comments in the main-methods.

https://chromiumcodereview.appspot.com/1677063003/diff/60001/tests/language/a...
File tests/language/async_star_no_cancel_test.dart (right):

https://chromiumcodereview.appspot.com/1677063003/diff/60001/tests/language/a...
tests/language/async_star_no_cancel_test.dart:40: subscription.cancel().then((_)
=> cancelFinished = true);
On 2016/02/10 21:13:17, hausner wrote:
> Maybe add and event here that proves that the subscription was cancelled. Then
> check for it below. That will catch if callbacks are not executed in the
> expected order. 

Done.

[floitsch, 2016-02-11 13:53:49]

Description was changed from

==========
More tests for async* functions.

An async* stream may only be canceled at yield-points.
==========

to

==========
More tests for async* functions.

An async* stream may only be canceled at yield-points.

R=hausner@google.com

Committed:
https://github.com/dart-lang/sdk/commit/310385aeb2b007774cf90dd6e965dfd2861f915a
==========

[floitsch, 2016-02-11 13:53:50]

Committed patchset #5 (id:80001) manually as
310385aeb2b007774cf90dd6e965dfd2861f915a (presubmit successful).

[hausner, 2016-02-11 16:49:15]

Nice, thank you.