Async Generator flow control tests #871

caitp · 2017-02-26T19:52:13Z

Here's a starter suite of tests for basic flow control in Async Generators.

This relates primarily to the behaviour of AsyncGeneratorResumeNext, AsyncGeneratorStart, AsyncGeneratorResolve and AsyncGeneratorReject, as well as some coverage of changes to GeneratorYield.

There is stuff missing, as I haven't exhaustively written tests for all of the variations that are necessary.

caitp · 2017-02-28T13:33:58Z

Ping, it would be great to get a look at these and start filling in holes in coverage here

leobalter · 2017-03-01T22:28:41Z

Hi, @caitp! I'm back to the regular work time on test262, although, there are other older PR I've had to look before this one.

I'll review this one first thing tomorrow.

Can you confirm if you're planning to add more tests in this PR or this is the current work that we should land and expect/start a follow PR for coverage completion?

caitp · 2017-03-01T22:39:54Z

well, if I come up with more tests, they'll be either in this PR or a followup. I don't think there's any problem with incrementally landing more coverage.

leobalter · 2017-03-01T22:45:16Z

It's not a problem, for sure. I just want to make the best time strategy for reviews and I really appreciate the work.

leobalter

There's a few nit picks, some related to formatting. It is looking great so far.

leobalter · 2017-03-02T17:28:56Z

test/language/expressions/async-generators/return-suspendedStart-promise.js

+---*/
+
+var g = async function*() {
+  $ERROR('Generator must not be resumed.');


we can replace $ERROR with throw new Test262Error('....

It should not block this PR, but it's a +1 if we can use the current conventional form.

leobalter · 2017-03-02T17:30:45Z

test/language/expressions/async-generators/expression-yield-promise.js

+/*---
+author: Caitlin Potter <[email protected]>
+esid: pending
+description: >


in every file, the description tags are in a good shape for the info tag. We still miss a short description for each case.

I'm open to suggestions on better description text, but otherwise I'm leaving these as-is, and the web runner can just deal with it

leobalter · 2017-03-02T17:36:21Z

test/language/expressions/async-generators/return-suspendedStart.js

+  // Generator is closed
+  it.next().then(function(ret) {
+    assert.sameValue(ret.value, undefined);
+    assert.sameValue(ret.done, true);


Interesting! You assert the iteration is done on line 22, but for this is still fine to assert the generator is really closed and won't resume. I like this.

+1

leobalter · 2017-03-02T18:00:42Z

test/language/expressions/async-generators/return-suspendedYield-try-finally-throw.js

+
+  it.return('sent-value').then($DONE, function(err) {
+    // Throw in finally block
+    assert(err instanceof Err);


if you store the reference for new Error you can throw it in the finally block and assert err is the same reference, right?

It also saves the need for an Error subclass

leobalter · 2017-03-02T18:01:23Z

test/language/expressions/async-generators/return-suspendedYield-try-finally.js

+
+  }).catch($DONE);
+
+}).catch($DONE);


I wish we could have Promise#finally already

I don't think it would help with the nested Promises

Is there a specific reason these are nested instead of chained anyway?

it.next() .then(function(ret) { assert.sameValue(ret.value, 1, 'Initial yield'); assert.sameValue(ret.done, false, 'Initial yield'); return it.return('sent-value'); }) .then(function(ret) { assert.sameValue(ret.value, 2, 'Yield in finally block'); assert.sameValue(ret.done, false, 'Yield in finally block'); return it.next(); }) .then(function(ret) { assert.sameValue(ret.value, 'sent-value', 'AsyncGeneratorResolve(generator, resultValue, true)'); assert.sameValue(ret.done, true, 'AsyncGeneratorResolve(generator, resultValue, true)'); return it.next(); }) .then(function(ret) { assert.sameValue(ret.value, undefined, 'Generator is closed'); assert.sameValue(ret.done, true, 'Generator is closed'); }) .then($DONE, $DONE);

(returning a Promise and chaining is) harder to read, less obvious, etc

I very much disagree, 4 level nested callbacks are much less readable than chaining promises, but I also don't care enough to block, it seems you've reached "annoyed" point on this PR (been there myself, sorry if I'm projecting) and I don't want to add to it any more. Thanks for the contribution!

leobalter · 2017-03-02T18:04:50Z

test/language/expressions/async-generators/throw-suspendedYield-try-finally.js

+  assert.sameValue(ret.value, 1);
+  assert.sameValue(ret.done, false);
+
+  it.throw(new Err).then(function(ret) {


this is the same case I mentioned above for storing an instance of new Error.

leobalter · 2017-03-02T18:08:25Z

test/language/expressions/async-generators/throw-suspendedYield.js

+it.next().then(function(ret) {
+  // Initial yield
+  assert.sameValue(ret.value, 1);
+  assert.sameValue(ret.done, false);


the comments are great to track the steps. If you don't mind, can you move them to become assertion messages?

This way we can have a powerful track of each assertion and, better, tells the implementors where each test is failing at.

Example for the case above:

assert.sameValue(ret.value, 1, 'Initial yield'); assert.sameValue(ret.done, false, 'Initial yield');

Even if they are the same message, I can already identify each one due to the respective expected values.

I've done this --- but it gets complicated for the generator-closing operations. I've gone with using the spec step which leads to the promise capability resolve/reject call instead there, but that seems awkward too.

leobalter · 2017-03-02T18:12:41Z

To avoid noises, I left some suggestions that can be applied to multiple places, but in a single comment as an example.

Some cases are:

Moving inline comments to assertion messages.
throwing the error reference to check for the sameValue instead of err instanceof Err, avoiding the Error subclass
splitting the current frontmatter description content into description and info

etc.

Thank you for the tests, @caitp! I'm looking forward the next review round!

tcare · 2017-03-03T20:26:45Z

Could you please keep spec text in an "info:" section and use "description:" as a short description of why the individual test is interesting?

leobalter · 2017-03-03T20:30:36Z

Thanks for the updates, @caitp.

As @tcare mentioned, the short description is something we need to keep the conventional conformance and a good point for a short reasoning on each test.

caitp · 2017-03-03T21:48:12Z

The description lines are influenced by existing tests in the repository, which include spec citations. I would appreciate specific suggestions for individual tests.

For example (test/language/expressions/arrow-function/syntax/arrowparameters-binding-identifier-eval.js):

// Copyright (C) 2015 the V8 project authors. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
es6id: 14.2.1
description: >
    ArrowParameters[Yield] :
      BindingIdentifier[?Yield]
      ...
    
    Parameter named "eval", non-strict

flags: [noStrict]
---*/
var af = eval => eval;


assert.sameValue(typeof af, "function");
assert.sameValue(af(1), 1);

tcare · 2017-03-03T23:49:07Z

Updating all the tests is preventatively expensive. Another example is the es6id there, which is now superseded by esid. We do drive-by cleanup and all new tests coming in should conform to the current standard.

caitp · 2017-03-04T00:27:13Z

I'm happy to change the frontmatter to anything requested (within reason), but please be specific about those requests.

tcare · 2017-03-04T02:32:33Z

Sorry for the confusion.

A random example: https://github.com/tc39/test262/blob/82c2ca070976329e822a466e2d6bafa88b87bc4d/test/intl402/PluralRules/prototype/select/tainting.js

Generally, "info:" can be used to dump spec text, and "description:" is for the brief description of what the test is trying to achieve or why it is interesting. We try to avoid having spec text in "description:" so that it can be used as a quick reference.

The descriptions from your previous commit 098f9ca are fine. Relevant spec text sections can be put into "info:".

leobalter · 2017-03-06T15:46:16Z

@caitp You've done a lot of great work here, and I really appreciate your contribution.

I will follow up with a new PR to address the remaining nit picky formatting things, mostly providing the frontmatter descriptions. Hopefully you may help me reviewing the new changes.

caitp force-pushed the AsyncGeneratorFlow branch from 82683c1 to 8bdcd58 Compare February 26, 2017 20:13

caitp mentioned this pull request Mar 1, 2017

test262 tests tc39/proposal-async-iteration#69

Open

gnarf added the needs review label Mar 2, 2017

leobalter self-assigned this Mar 2, 2017

leobalter suggested changes Mar 2, 2017

View reviewed changes

leobalter added awaiting author and removed needs review labels Mar 2, 2017

Async Generator flow control tests

79baf2b

caitp force-pushed the AsyncGeneratorFlow branch from 8bdcd58 to 79baf2b Compare March 3, 2017 18:28

leobalter approved these changes Mar 6, 2017

View reviewed changes

leobalter merged commit 4914e47 into tc39:master Mar 6, 2017

leobalter mentioned this pull request Mar 6, 2017

Fix frontmatter description/info tags for Async generators flow control tests #889

Closed

Async Generator flow control tests #871

Async Generator flow control tests #871

Uh oh!

Conversation

caitp commented Feb 26, 2017

Uh oh!

caitp commented Feb 28, 2017

Uh oh!

leobalter commented Mar 1, 2017

Uh oh!

caitp commented Mar 1, 2017

Uh oh!

leobalter commented Mar 1, 2017

Uh oh!

leobalter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caitp Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnarf Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobalter commented Mar 2, 2017

Uh oh!

tcare commented Mar 3, 2017

Uh oh!

leobalter commented Mar 3, 2017

Uh oh!

caitp commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcare commented Mar 3, 2017

Uh oh!

caitp commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcare commented Mar 4, 2017

Uh oh!

leobalter commented Mar 6, 2017

Uh oh!

Uh oh!

caitp Mar 3, 2017 •

edited

Loading

gnarf Mar 6, 2017 •

edited

Loading

caitp commented Mar 3, 2017 •

edited

Loading

caitp commented Mar 4, 2017 •

edited

Loading