feat: adding tests for optional chaining proposal #2212

bcoe · 2019-06-24T03:19:05Z

starting to work on a few tests for optional chaining as I read and review the proposal:

Punctuators

?.[lookahead ∉ DecimalDigit] link.

Left-Hand-Side Expressions

OptionalExpression:

MemberExpression OptionalChain
CallExpression OptionalChain
OptionalExpression OptionalChain

OptionalChain:

?. [ expression ]
?. IdentifierName
?. Arguments
?. TemplateLiteral (this production is an early error). link
OptionalChain [ expression ]
OptionalChain IdentifierName
OptionalChain Arguments
OptionalChain TemplateLiteral (this production is an early error).

Static Semantics: IsValidSimpleAssignmentTarget

LeftHandSideExpression: OptionalExpression link.

bcoe · 2019-06-24T04:13:40Z

test/language/optional-chaining/destructuring.js

+description: >
+  Static Semantics: IsDestructuring
+    LeftHandSideExpression:
+      CallExpression


@jridgewell @DanielRosenwasser @rkirsling, I read this rule as stating that destructuring is not supported in an optional chain, I also gathered that this was the outcome of:

tc39/proposal-optional-chaining#74

This test is failing because the current babel implementation supports destructuring.

Am I correct in my reading of this part of the proposal, if so I think we'll want to update the babel implementation.

Hmm, it seems that IsDestructuring is only checked from ForIn/OfBodyEvaluation, and I think we're just saying that if you see an OptionalExpression as an LHS, then (just like a CallExpression) that's not an example of destructuring.

So I don't know that there's anything that needs to be tested here—note that this test doesn't actually involve an optional chain at present, and the linked idea about optional destructuring is something that would require additional syntax.

@rkirsling I understand, it's the concept of "optional destructuring" that's out of scope, and the proposal was something along the lines of foo?.bar?; my bad misreading that section of the spec.

leobalter · 2019-06-24T12:21:04Z

I recommend you create a checklist of all the new productions possible by the new syntax rather than having just generic tests. Perhaps, one test here and there might work nice for feature check but there is still a big chunk of missing coverage here.

bcoe · 2019-06-27T20:23:51Z

@leobalter I fleshed out the issue body, based on your recommendations. Is that looking like what you were picturing, if so, any recommendations for naming conventions of tests?

test/language/expressions/optional-chaining/early-errors-tail-position-template-string.js

test/language/expressions/optional-chaining/member-expression-optional-chain.js

leobalter

There are a lot of missing parts here even if the checklist seems like a good start.

The tests need to expand the syntactical productions rather than just picking one for each. This also miss other cases with the await and yield positions flagged in here.

Also: chaining the OptionalChain:

OptionalChain: OptionalChain [Expression]
OptionalChain . IdentifierName
OptionalChain Arguments
OptionalChain TemplateLiteral

and mixing these together too.

There is also a need to verify the grammar of the OptionalExpression into parts receiving a LeftHandSideExpression, like e.g. LeftHandSideExpression AssignmentOperator AssignmentExpression.

It also needs expansion for cases like

MemberExpression OptionalChain
CallExpression OptionalChain
OptionalExpression OptionalChain

expanding MemberExpression and CallExpression until a fair amount of cases.

I believe this will be a much longer work as it usually is for new syntax features. I'm not sure if we should start so much effort before Stage 3 for this feature as well. I'd rather wait a bit until the proposal finds a better stability.

test/language/expressions/optional-chaining/member-expression-optional-chain.js

+};
+
+// OptionalChain: ?.[Expression]
+assert.sameValue(11, arr?.[i + 1]);


test/language/expressions/optional-chaining/member-expression-optional-chain.js

+assert.sameValue(11, arr?.[i + 1]);
+
+// OptionalChain: ?.IdentifierName
+assert.sameValue('hello', obj?.a);


leobalter · 2019-07-03T21:28:15Z

test/language/expressions/optional-chaining/member-expression-optional-chain.js

+assert.sameValue('hello', obj?.a);
+
+// OptionalChain: ?.Arguments
+assert.sameValue(30, fn?.(10, 20));


And Arguments need to be expanded to more productions too. There are some cases and templates from the test generation parts that could be reused for these.

leobalter · 2019-07-03T21:29:22Z

test/language/expressions/optional-chaining/member-expression-optional-chain.js

+assert.sameValue(30, fn?.(10, 20));
+
+// OptionalChain: OptionalChain [Expression]
+assert.sameValue(12, obj?.arr[i + 1]);


Once again, double exposition of Expression when you have [Expression][Expression] and all the multi combinations you can get here, same will go for the chain of possible productions.

leobalter · 2019-07-03T21:30:03Z

test/language/expressions/optional-chaining/optional-expression-optional-chain.js

+info: |
+  optional chain on recursive optional expression
+description: >
+  Left-Hand-Side Expressions


the content for info and description are inverted in the test files.

test/language/expressions/optional-chaining/runtime-semantics-evaluation.js

leobalter · 2019-07-03T21:51:00Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string.js

+const a = {fn() {}};
+
+a?.fn
+  `hello`;


why not having expanding this to verify

a?.fn`hello`;

in the same line?

I was specifically trying to address this section of the spec:

NOTE This production exists in order to prevent automatic semicolon insertion rules (11.9) to be applied to the following code.

... however, by adding a semi-colon to the end of the expression I was failing to actually exercise this behavior, I've split it into another test.

thanks for the clarification

leobalter · 2019-07-03T21:52:09Z

test/language/expressions/optional-chaining/call-expression-optional-chain.js

+
+const fn = () => {
+  return {a: 33};
+};


Nit:

function fn() { return {a: 33}; }

leobalter · 2019-07-03T21:53:15Z

test/language/expressions/optional-chaining/call-expression-optional-chain.js

+};
+
+assert.sameValue(33, fn()?.a);
+assert.sameValue(undefined, fn()?.b?.c);


does this need to go to c? Could definitely sport replacing it with a throwing function call, or even ?.a after ?.b

test/language/expressions/optional-chaining/short-circuiting.js

bcoe · 2019-07-04T06:36:08Z

@leobalter I offered to review the spec text of optional chaining, as part of the entrance criteria for stage 3.

Writing tests has been helping me better dive into edge-cases of the proposal, and get a better feel for the specification.

I'd love to help see these tests further along, as it's a great learning experience. Would you rather that I close this draft until we've reached stage 3 approval? or is it okay if I continue putting some work into it.

leobalter · 2019-07-05T14:53:38Z

Closing is not necessary but we won't be able to merge the tests until it reaches stage 3.

I'm ok to leave this open, including this new "Draft" option in the PR as a good flag. As I pointed out there is a good chunk of coverage to be done yet. I might wait until Stage 3 for another review, but feel free to ping me for any questions.

bcoe · 2019-07-13T00:15:57Z

@leobalter I made an effort to enumerate more test cases today, reading through the spec. Let me know if this is looking more like it.

https://gist.github.com/bcoe/b0e6ae3f93f8c8b8f8559f4428187f25

Note, I put proposed file-names for the tests to the right of the productions.

leobalter

There are some few changes to let us land this PR. We can work on the extended coverage checklist through follow up PRs.

leobalter · 2019-08-01T19:07:38Z

test/language/expressions/optional-chaining/call-expression-optional-chain.js

@@ -0,0 +1,17 @@
+/*---


Can you add the copyright headers into each file? They are required to part tests correctly in different test runners used to consume Test262.

leobalter · 2019-08-01T19:08:30Z

test/language/expressions/optional-chaining/call-expression-optional-chain.js

@@ -0,0 +1,17 @@
+/*---
+esid: pending


please use esid: prod-OptionalExpression instead of any pending in the test files.

leobalter · 2019-08-01T19:09:43Z

test/language/expressions/optional-chaining/call-expression-optional-chain.js

+esid: pending
+info: |
+  optional chain on call expression
+description: >


invert usage of info and description. Description is the small line, info is the large chunk.

You may keep the block characters info: | (pipe) and description: > (greater than)

bcoe · 2019-08-01T20:43:12Z

@leobalter I have a fairly light weekend, I'll make an effort to put in a few hours expanding the cases outlined in the gist.

leobalter · 2019-08-01T20:54:14Z

@leobalter I have a fairly light weekend, I'll make an effort to put in a few hours expanding the cases outlined in the gist.

Sure! If you can first address the small items in here, I can already land these tests. I've wrote some other tests (related) to #2262 and it's just a very small chunk from the complete coverage.

leobalter

The tests in this PR are correct. I have some follow ups I described in the comments.

leobalter · 2019-08-05T14:48:45Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string-esi.js

+
+$DONOTEVALUATE();
+
+const a = {fn() {}};


this can be a simple var a;. To enable the test, all you need is to have a valid reference for a.

leobalter · 2019-08-05T14:49:33Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string-esi.js

+  Static Semantics: Early Errors
+    OptionalChain:
+      ?.TemplateLiteral
+      OptionalChain TemplateLiteral


please add the error part: It is a Syntax Error if any code matches this production.

leobalter · 2019-08-05T14:51:19Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string-esi.js

+// This production exists in order to prevent automatic semicolon
+// insertion rules.
+a?.fn
+  `hello`


For a follow up PR: we are missing tests for the ?.TemplateLiteral productions:

a?.`hello` // and a?. `hello`

Also verify this is valid syntax:

`foo`?.bar

leobalter · 2019-08-05T14:52:05Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string.js

+  Static Semantics: Early Errors
+    OptionalChain:
+      ?.TemplateLiteral
+      OptionalChain TemplateLiteral


please add the error part: It is a Syntax Error if any code matches this production.

leobalter · 2019-08-05T14:58:35Z

test/language/expressions/optional-chaining/call-expression.js

+
+// CallExpression Arguments
+assert.sameValue(33, fn()?.a);
+assert.sameValue(undefined, fn()?.b);


in a follow up PR, we should expand these to other types and checking property existence:

Currently the CallExpression evaluates to:

An Object with the property a

Missing:

The CallExpression evaluates to falsy values
a. false
b. 0
c. ''
d. undefined // different behavior
e. null // same as undefined

CallExpression evaluates to other type values.
a. true
b. 1
c. NaN
d. 'undefined'
e. Symbol

Verify [[Get]] from the returned object properties

This test itself is good enough as a syntax test, these expansions are more useful for further evaluation of the results.

leobalter · 2019-08-05T14:59:13Z

test/language/expressions/optional-chaining/early-errors-tail-position-template-string.js

+
+$DONOTEVALUATE();
+
+const a = {fn() {}};


name can be simplified here as well.

leobalter · 2019-08-05T15:07:56Z

test/language/expressions/optional-chaining/runtime-semantics-evaluation.js

+
+const nul = null;
+const undf = undefined;
+assert.sameValue(undefined, nul?.a);


for a follow up:

var x = 0; var y = null?.[(function() { x += 1 }())]; // assert value of x to verify if the `[Expression]` has got evaluated // assert y is undefined.

oh! you did that in test/language/expressions/optional-chaining/short-circuiting.js, nice!

leobalter · 2019-08-05T15:09:13Z

test/language/expressions/optional-chaining/static-semantics-simple-assignment.js

+
+const obj = {};
+
+obj?.a = 33;


bcoe · 2019-08-05T20:30:54Z

@leobalter 🎉 I have this PR open on my fork of test262, which I've been adding a more thorough set of grammar productions to.

I will make this PR official now that this has landed, and address your comments in it.

bcoe commented Jun 24, 2019

View reviewed changes

bcoe force-pushed the optional-chaining-tests branch from 28cfd4f to c53665b Compare July 1, 2019 01:08

rwaldron reviewed Jul 2, 2019

View reviewed changes

test/language/expressions/optional-chaining/early-errors-tail-position-template-string.js Outdated Show resolved Hide resolved

rwaldron reviewed Jul 2, 2019

View reviewed changes

test/language/expressions/optional-chaining/member-expression-optional-chain.js Outdated Show resolved Hide resolved

leobalter suggested changes Jul 3, 2019

View reviewed changes

leobalter mentioned this pull request Jul 30, 2019

Add tests for optional chaining #2263

Closed

leobalter self-assigned this Jul 30, 2019

leobalter suggested changes Aug 1, 2019

View reviewed changes

bcoe marked this pull request as ready for review August 2, 2019 23:19

bcoe added 8 commits August 4, 2019 22:55

feat: adding tests for optional chaining proposal

8d67614

test: add tests for assignment and destructuring

2a26709

chore: update test

1ecf3ee

test: this test was based on incorrect reading of spec

12d1af1

test: continuing to flesh out test cases

9d53a4b

chore: add additional tests

13d9cb0

chore: slight tweak to formatting

4e81676

chore: address code review

bc393df

bcoe force-pushed the optional-chaining-tests branch from 7081b52 to bc393df Compare August 5, 2019 05:55

lint: address linting errors

129acbb

leobalter approved these changes Aug 5, 2019

View reviewed changes

leobalter merged commit aae991d into tc39:master Aug 5, 2019

DanielRosenwasser mentioned this pull request Oct 31, 2019

Normative: Add Optional Chaining tc39/ecma262#1646

Merged


		$DONOTEVALUATE();

		const a = {fn() {}};


		$DONOTEVALUATE();

		const a = {fn() {}};

feat: adding tests for optional chaining proposal #2212

feat: adding tests for optional chaining proposal #2212

Conversation

bcoe commented Jun 24, 2019 • edited Loading

Punctuators

Left-Hand-Side Expressions

Static Semantics: IsValidSimpleAssignmentTarget

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Jun 24, 2019

bcoe commented Jun 27, 2019

leobalter left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

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

bcoe commented Jul 4, 2019

leobalter commented Jul 5, 2019

bcoe commented Jul 13, 2019 • edited Loading

leobalter 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

bcoe commented Aug 1, 2019

leobalter commented Aug 1, 2019

leobalter 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

bcoe commented Aug 5, 2019

bcoe commented Jun 24, 2019 •

edited

Loading

bcoe commented Jul 13, 2019 •

edited

Loading