#1363. Tests for yield updated to the current specification #1370

sgrekhov · 2022-07-14T18:33:03Z

No description provided.

…ification

alexmarkov

The new tests lgtm (modulo a couple of comments).

However, I think it would be good if @eernstg or @lrhn would review and approve these tests.

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t01.dart

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t03.dart

eernstg

LGTM, but please consider the following as well:

A couple of things are worth considering: In several situations there is an ss.resume() followed by code which is intended to check that certain lists contain the expected elements. However, it looks like there should be a suspension point before those checks, because otherwise the generator doesn't get a chance to do anything wrong. So maybe await null; should be added?

Another thing to consider is that delays will make the trybots/buildbots run more slowly. I suspect that it will work just as well to have a simple await null; in several situations than it does to have a delay of 100 milliseconds. Waiting for an entire second is of course even more disruptive to the bot execution. We could have several await null;, too.

eernstg · 2022-07-22T21:42:03Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t02.dart

+      Expect.listEquals([1], readyToSend);
+      ss.resume();
+      await Future.delayed(Duration(milliseconds: 100));
+      Expect.listEquals([1, 2, 3], sent);


This looks dangerous: I don't think it would be a bug for an implementation to somehow fail to put all those elements into sent, in spite of the fact that it is probably extremely unlikely.

Perhaps just test that whatever elements are there is a prefix of the whole list?

Changed to

// `sent` may contain not all elements of the stream sent.forEach((element) { Expect.isTrue([1, 2, 3].contains(element)); });

eernstg · 2022-07-22T22:04:57Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t08.dart

+      Expect.listEquals([1, 2], readyToSend);
+      Expect.isTrue(sc.isPaused);
+      ss.resume();
+      await Future.delayed(Duration(milliseconds: 100));


Same issue as before: In principle, we can't know that other microtasks have actually been scheduled just because there was a delay.

The delay is a timer-delay, so we know that all microtask-based events have been processed.
I'm not sure we promise that buffered non-sync-controller events are delivered using microtasks, but if they're not, it'd probably be zero-timer events.
So, this likely works.

eernstg · 2022-07-22T22:09:39Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A03_t01.dart

+    ss.cancel();
+    if (!c.isCompleted) {
+      c.complete();
+    }


Should it not be safe to replace line 49,50,51 by c.complete()? How would it fail?

Indeed. Fixed. Thank you

eernstg · 2022-07-22T22:12:34Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A03_t02.dart

+    await ss.cancel();
+    if (!c.isCompleted) {
+      c.complete();
+    }


Same as previous test: Why not have only c.complete()?

eernstg · 2022-07-22T22:15:04Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A03_t03.dart

+  late StreamSubscription<int> ss;
+  ss = s.listen((int x) async {
+    if (!generatorAlive) {
+      Expect.fail("Generator function must be terminated");


Probably Expect.fail("The generator function is unexpectedly not alive") would be easier to understand.

eernstg · 2022-07-22T22:15:48Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A03_t03.dart

+      Expect.fail("Generator function must be terminated");
+    }
+    if (x == 5) {
+      // let generator to work some time


Perhaps // Allow the generator to work for some time.

Changed everywhere

eernstg · 2022-07-22T22:16:32Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A03_t03.dart

+    }
+    if (x == 5) {
+      // let generator to work some time
+      Expect.equals(true, generatorAlive);


Why not Expect.isTrue(generatorAlive)?

eernstg · 2022-07-22T22:25:41Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A04_t01.dart

+  finish = false;
+  Stream<int> s = generator();
+  s.listen((int x) async {
+    finish = (x > 10); // let generator to work some time


// Allow the generator....

eernstg · 2022-07-22T22:27:37Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A04_t02.dart

+  finish = false;
+  Stream<int> s = generator();
+  s.listen((int x) {
+    finish = (x > 10); // let generator to work some time


eernstg · 2022-07-22T22:41:23Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A05_t01.dart

+  late StreamSubscription<int> ss;
+  ss = s.listen((int i) async {
+    // This 'await' returns control back to event loop and generator goes on
+    await Future.delayed(Duration(milliseconds: 100));


I tried to replace this by await null; await null; await null;, and it works fine. ;-)

Replacing it by await null; await null; does not work, and this illustrates that it is directly connected to the number of other tasks we allow the scheduler to handle (except that we're not supposed to assume that we can control the scheduler in any precise manner by doing these things).

…placed by `await null;`

sgrekhov · 2022-07-25T09:30:09Z

In several situations there is an ss.resume() followed by code which is intended to check that certain lists contain the expected elements. However, it looks like there should be a suspension point before those checks, because otherwise the generator doesn't get a chance to do anything wrong. So maybe await null; should be added?

If to add a suspension point after the resume it'll chane the expected behavior. Please see dart-lang/sdk#49465 So, when there is no such point it usually means "resume, make sure that nothing was changed immediately after the resume, then check something more meaningful". If you find this check, immediately after resume(), excessive, I don't mind to remove it

Another thing to consider is that delays will make the trybots/buildbots run more slowly. I suspect that it will work just as well to have a simple await null; in several situations than it does to have a delay of 100 milliseconds. Waiting for an entire second is of course even more disruptive to the bot execution. We could have several await null;, too.
I think it's safe to use await null; after pause(); and then check that stream is really paused. Changet it to await null; in such places. Thank you.

Please rereview

eernstg

LGTM!

I didn't understand that the checks after resume were intended to check the situation without any asynchronous scheduling events. When that's the purpose it is fine.

@sgrekhov

…art-lang#1370) Authored by @sgrekhov. Tests for yield fixed and updated to the current specification. Completer added to avoid flakiness. Missing asyncStart()/asyncEnd() added. Some delays replaced by `await null;`.

lrhn

drive-by comments.

lrhn · 2022-08-09T13:53:02Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A05_t03.dart

+    if (i == 2) {
+      Expect.listEquals([1], sent);
+      Expect.listEquals([1, 2], readyToSend);
+      ss.resume();


Not sure what ss.resume() should do. The subscription is never paused, so it should be a no-op.
(But if you are just testing that it's a no-op, then it's correct.)

Description of this test

Check that resuming of the [Stream] which was not paused doesn't affect anything

So yes, it's testing that it's no-op

lrhn · 2022-08-09T13:55:53Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t02.dart

+      await Future.delayed(Duration(milliseconds: 100));
+      // `sent` may contain not all elements of the stream
+      sent.forEach((element) {
+        Expect.isTrue([1, 2, 3].contains(element));


Seems like a long way of saying element <= 3.
Not a very telling test - is it possible to fail it at all?

Indeed. Thanks! Please see #1390

lrhn · 2022-08-09T13:57:15Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t03.dart

+      await ss.cancel();
+    }
+  });
+  await Future.delayed(Duration(seconds: 1));


A very long delay, where seconds: 0 would likely give precisely the same result because everything else is microtask based.

(The next variant, using a Completer, seems much closer to what I'd write for this test.)

The latest version of this test doen't contain this long delay and is based on a Completer. Please see https://github.com/dart-lang/co19/blob/master/Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t03.dart

lrhn · 2022-08-09T14:07:06Z

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t08.dart

+      Expect.listEquals([1, 2], readyToSend);
+      Expect.isTrue(sc.isPaused);
+      ss.resume();
+      await Future.delayed(Duration(milliseconds: 100));


The delay is a timer-delay, so we know that all microtask-based events have been processed.
I'm not sure we promise that buffered non-sync-controller events are delivered using microtasks, but if they're not, it'd probably be zero-timer events.
So, this likely works.

2022-08-04 sgrekhov22@gmail.com dart-lang/co19#993. FFI tests refactored and improved (dart-lang/co19#1387) 2022-08-04 sgrekhov22@gmail.com dart-lang/co19#993. [FFI] Missed tests added for some new types (dart-lang/co19#1386) 2022-08-03 sgrekhov22@gmail.com Fixes dart-lang/co19#1129. Don't expect any proxy routine in case of DIRECT connection (dart-lang/co19#1383) 2022-08-03 sgrekhov22@gmail.com dart-lang/co19#993. [FFI] Missed tests added for some new types (dart-lang/co19#1384) 2022-07-29 sgrekhov22@gmail.com dart-lang/co19#673. IFrame tests that don't actually test IFrame removed (dart-lang/co19#1382) 2022-07-29 sgrekhov22@gmail.com dart-lang/co19#1363. Some delays removed or reduced (dart-lang/co19#1380) 2022-07-29 sgrekhov22@gmail.com dart-lang/co19#1363 Async `for-in` test updated according to the current specification (dart-lang/co19#1379) 2022-07-27 sgrekhov22@gmail.com dart-lang/co19#1363. Yield each tests updated to the current specification (dart-lang/co19#1374) 2022-07-27 sgrekhov22@gmail.com Fixes dart-lang/co19#1375. Add test for final variable in a for-in loop (dart-lang/co19#1378) 2022-07-25 sgrekhov22@gmail.com dart-lang/co19#1363. Tests for yield updated to the current specification (dart-lang/co19#1370) Change-Id: Ie92f4b2157284777177012320883247411e214fa Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254140 Reviewed-by: Erik Ernst <eernst@google.com> Commit-Queue: Alexander Thomas <athom@google.com>

dart-lang#1363. Tests for yield updated to the current specification

7ce3da8

sgrekhov requested review from eernstg and alexmarkov July 14, 2022 18:33

sgrekhov mentioned this pull request Jul 14, 2022

Wrong behavior of yield when associated stream is cancelled dart-lang/sdk#49451

Closed

sgrekhov added 2 commits July 15, 2022 16:25

dart-lang#1363. Tests for yield fixed and updated to the current spec…

2dcf529

…ification

dart-lang#1363. Tests for yield fixed and updated to the current spec…

612e8b4

…ification

sgrekhov requested a review from chloestefantsova July 18, 2022 10:24

alexmarkov reviewed Jul 18, 2022

View reviewed changes

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t01.dart Outdated Show resolved Hide resolved

Language/Statements/Yield_and_Yield_Each/Yield/execution_async_A01_t03.dart Show resolved Hide resolved

sgrekhov added 3 commits July 19, 2022 10:17

dart-lang#1363. Typo fixed, completer added to avoid flakyness

a6c1a66

dart-lang#1363. Missed asyncStart()/asyncEnd() added

1d72fa1

dart-lang#1363. Missed test for resume() with no pause() added

d23f79f

eernstg approved these changes Jul 22, 2022

View reviewed changes

sgrekhov added 2 commits July 25, 2022 12:01

dart-lang#1363. Changes according to the review notes

e62d192

dart-lang#1363. Changes according to the review notes. Some delays re…

f3038a1

…placed by `await null;`

sgrekhov requested a review from eernstg July 25, 2022 09:30

eernstg approved these changes Jul 25, 2022

View reviewed changes

eernstg merged commit acc78f9 into dart-lang:master Jul 25, 2022

lrhn reviewed Aug 9, 2022

View reviewed changes

sgrekhov deleted the co19-1363 branch September 29, 2022 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1363. Tests for yield updated to the current specification #1370

#1363. Tests for yield updated to the current specification #1370

sgrekhov commented Jul 14, 2022

alexmarkov left a comment

eernstg left a comment

eernstg Jul 22, 2022

sgrekhov Jul 25, 2022

eernstg Jul 22, 2022

lrhn Aug 9, 2022

eernstg Jul 22, 2022

sgrekhov Jul 25, 2022

eernstg Jul 22, 2022

eernstg Jul 22, 2022

sgrekhov Jul 25, 2022

eernstg Jul 22, 2022

sgrekhov Jul 25, 2022

eernstg Jul 22, 2022

sgrekhov Jul 25, 2022

eernstg Jul 22, 2022

eernstg Jul 22, 2022

eernstg Jul 22, 2022

sgrekhov commented Jul 25, 2022

eernstg left a comment

lrhn left a comment

lrhn Aug 9, 2022

sgrekhov Aug 10, 2022

lrhn Aug 9, 2022

sgrekhov Aug 10, 2022

lrhn Aug 9, 2022

sgrekhov Aug 10, 2022

lrhn Aug 9, 2022

#1363. Tests for yield updated to the current specification #1370

#1363. Tests for yield updated to the current specification #1370

Conversation

sgrekhov commented Jul 14, 2022

alexmarkov left a comment

Choose a reason for hiding this comment

eernstg left a 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

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

sgrekhov commented Jul 25, 2022

eernstg left a comment

Choose a reason for hiding this comment

lrhn left a 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