Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: adding async optional chaining tests #2337

Merged
merged 12 commits into from
Sep 18, 2019

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 8, 2019

@bcoe bcoe force-pushed the async-optional-chaining branch from 0a547a5 to 434ca29 Compare September 8, 2019 15:07
bcoe and others added 11 commits September 16, 2019 22:32
…nc-optional-chain-square-brackets.js

Co-Authored-By: Leo Balter <[email protected]>
…nc-optional-chain-square-brackets.js

Co-Authored-By: Leo Balter <[email protected]>
…nc-optional-chain-square-brackets.js

Co-Authored-By: Leo Balter <[email protected]>
@bcoe
Copy link
Contributor Author

bcoe commented Sep 17, 2019

@leobalter I've updated the tests, I'm not following this one production however:

  Promise.prototype.y = 43;
  var res = await Promise.reject(undefined)?.y;
  assert.sameValue(res, 43, 'await unwraps the evaluation of the whole optional chaining expression #2');

I believe this would result in an exception being thrown, not 43 being resolved; can we just drop it?

@leobalter
Copy link
Member

@leobalter I've updated the tests, I'm not following this one production however:

  Promise.prototype.y = 43;
  var res = await Promise.reject(undefined)?.y;
  assert.sameValue(res, 43, 'await unwraps the evaluation of the whole optional chaining expression #2');

I believe this would result in an exception being thrown, not 43 being resolved; can we just drop it?

await does not unwrap the rejected promise, that's the tricky part. The optional chaining will evaluate the Promise.reject(undefined) to an instance of Promise, than it gets the y property. the await expression will never see the rejected promise there.

This might illustrate better the same resolution:

var prom = Promise.reject(undefined);
var res = await (prom?.y);

// or

var evaluation = prom?.y;
var res = await evaluation;

The test case is interesting as it verifies the order for runtime evaluation. We can add this in a follow up too.

Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applying the mentioned "reject to chain" part in a follow up.

It's applied already. No need to follow up.

@leobalter leobalter merged commit c41a8ac into tc39:master Sep 18, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Sep 18, 2019

@leobalter my concern was, at least in Node.js:

var res = await Promise.reject(undefined)?.y;

bubbles to an unhandled rejection and fails the test.

@leobalter
Copy link
Member

that doesn't make the test incorrect, right? We can't block tests because a specific host has a non specific behavior, and I believe Node might have some flag to ignore non handled rejections. In this case, Node is even doing the correct evaluation but the warning is triggering a test failure in some test262 runner.

It's important to have these tests, and I believe this is not gonna be the first test here with this rejection swallowing.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 27, 2019

@leobalter I trust you 😄, was mainly concerned I was unable to validate in the test runner I had pulled together.

@bcoe bcoe deleted the async-optional-chaining branch September 27, 2019 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants