#1363. Yield each tests updated to the current specification #1374
Conversation
There was a problem hiding this comment.
LGTM, noting a few things.
The most delicate point is that the tests have a built-in assumption about the execution of asynchronous work (that is, a specific action is expected to have taken place because there was an await with a delay of 100ms, or something like that). I think this is safe in practice, and it might not be practical to express the constraint which is actually safe to assume (which would allow for a large number of different orderings of events), so I guess we'll have to have these tests where the outcome is something like [-1, 1, 'one', ...] even though there is no guarantee that things are actually going to happen in exactly that order.
Another thing to consider is that there are several cases where sc.close() is executed rather than await sc.close(). This may be fine as long as the close operation is performed after main has returned, but when it is not the last operation in main it looks like it might not establish the intended situation. So please take a look at all occurrences of sc.close(), and see if each of them is known to do the right thing.
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t01.dart
Outdated
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A02_t01.dart
Outdated
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t07.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t08.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t08.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t12.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t10.dart
Outdated
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t11.dart
Outdated
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t12.dart
Outdated
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t13.dart
Outdated
Show resolved
Hide resolved
|
@alexmarkov, it would be great if you could take a look, too! |
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t11.dart
Show resolved
Hide resolved
| }); | ||
| await c.future; | ||
| sc.add(1); | ||
| await new Future.delayed(new Duration(milliseconds: 100)); |
There was a problem hiding this comment.
Can we avoid these hardcoded delays? They may artificially increase execution time of the test, and at the same time these delays don't guarantee that anything will run during this time period if there is a high load on the bot, so we may end up with flaky failures.
There was a problem hiding this comment.
Yes, tests rewritten to not to use these delays
…lays but use a completer
|
@eernstg @alexmarkov thank you for the review. I rewrote the tests so they don't rely on delays any longer but use |
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t07.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t08.dart
Show resolved
Hide resolved
Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_A03_t11.dart
Show resolved
Hide resolved
…dart-lang#1374) Authored by @sgrekhov. Yield-each tests updated and new ones added; some delays replaced by `await null;`; do not to rely on delays, but use a completer (to avoid having flakiness during high loads, and in general to avoid depending on scheduling order and timing).
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>
No description provided.